Moodle
  1. Moodle
  2. MDL-29594

Course category tree on frontpage costs a db query per course

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.1.1, 2.7
    • Fix Version/s: BACKEND
    • Component/s: Performance
    • Labels:

      Description

      If the course category tree is used it costs a db query per course, this come from lots of enrol queries generated from the call to:

      enrol_get_course_info_icons()

      in course/renderer.php function course_category_tree_category()

      This means that page doesn't scale well if the site has thousands of courses

        Gliffy Diagrams

          Issue Links

            Activity

            Dan Poltawski created issue -
            Michael de Raadt made changes -
            Field Original Value New Value
            Assignee moodle.com [ moodle.com ] Sam Marshall [ quen ]
            Component/s Navigation [ 10599 ]
            Hide
            Michael de Raadt added a comment -

            Sorry, I clicked on the wrong Sam.

            Show
            Michael de Raadt added a comment - Sorry, I clicked on the wrong Sam.
            Michael de Raadt made changes -
            Assignee Sam Marshall [ quen ] Sam Hemelryk [ samhemelryk ]
            Hide
            Sam Hemelryk added a comment -

            Marking this triaged, confirmed it will in deed still be bad for performance.
            Perhaps a great candidate for MUC.

            Many thanks
            Sam

            Show
            Sam Hemelryk added a comment - Marking this triaged, confirmed it will in deed still be bad for performance. Perhaps a great candidate for MUC. Many thanks Sam
            Sam Hemelryk made changes -
            Fix Version/s STABLE backlog [ 10463 ]
            Labels triaged
            Assignee Sam Hemelryk [ samhemelryk ] moodle.com [ moodle.com ]
            Component/s Navigation [ 10599 ]
            Hide
            Sam Hemelryk added a comment -

            Linking to the MUC stage 2 issue as this is probably low hanging fruit.

            Show
            Sam Hemelryk added a comment - Linking to the MUC stage 2 issue as this is probably low hanging fruit.
            Sam Hemelryk made changes -
            Link This issue has a non-specific relationship to MDL-34224 [ MDL-34224 ]
            Sam Hemelryk made changes -
            Assignee moodle.com [ moodle.com ] Sam Hemelryk [ samhemelryk ]
            Hide
            Sam Hemelryk added a comment -

            Putting this up for peer-review.
            The solution I am purposing simply introduces a cache to store the icons for a course (as an array of objects).
            It is pretty simple + safe and if we hit the cache we avoid the need to include enrollment plugins and save ourselves 1 query per course.

            Show
            Sam Hemelryk added a comment - Putting this up for peer-review. The solution I am purposing simply introduces a cache to store the icons for a course (as an array of objects). It is pretty simple + safe and if we hit the cache we avoid the need to include enrollment plugins and save ourselves 1 query per course.
            Sam Hemelryk made changes -
            Status Open [ 1 ] Waiting for peer review [ 10012 ]
            Pull Master Diff URL https://github.com/samhemelryk/moodle/compare/master...wip-MDL-29594-m25
            Pull Master Branch wip-MDL-29594-m25
            Pull from Repository git://github.com/samhemelryk/moodle.git
            Hide
            Sam Hemelryk added a comment -

            Hi Petr,

            I know you are busy but you know the enrolment code better than anyone.
            Would you mind having a look at the cache solution I am purposing and seeing what you think.

            Many thanks
            Sam

            Show
            Sam Hemelryk added a comment - Hi Petr, I know you are busy but you know the enrolment code better than anyone. Would you mind having a look at the cache solution I am purposing and seeing what you think. Many thanks Sam
            Hide
            Dan Poltawski added a comment -

            I had a look at this and it looks nice I don't know enough about other enrollment things to say confidently that we don't need to invlidate elsewhere.

            Show
            Dan Poltawski added a comment - I had a look at this and it looks nice I don't know enough about other enrollment things to say confidently that we don't need to invlidate elsewhere.
            Hide
            Petr Skoda added a comment -

            This looks like a big change of API to me, previously everybody could see a different set of API (enrolled x not enrolled, completed, enrolment period open/closed, etc.) after this change the icons would be frozen.

            Maybe there could be created a general enrol instance caching instead because there are several other places that fetch data from enrol table.

            My -1 for current patch, sorry.

            Show
            Petr Skoda added a comment - This looks like a big change of API to me, previously everybody could see a different set of API (enrolled x not enrolled, completed, enrolment period open/closed, etc.) after this change the icons would be frozen. Maybe there could be created a general enrol instance caching instead because there are several other places that fetch data from enrol table. My -1 for current patch, sorry.
            Sam Hemelryk made changes -
            Status Waiting for peer review [ 10012 ] Development in progress [ 3 ]
            Hide
            Sam Hemelryk added a comment -

            Thanks for looking fella's.

            Re: enrollment state + statuses, I was concerned that may be the case.
            I'd had a quick scan through the code and couldn't find anywhere those were being factored in with the icons but I figured they'd be somewhere.
            Not to mention I suppose we can't be sure that the plugins aren't delivering user specific icons as well I suppose.

            No probs, this was a quick knock up. I'll invest more time and look to identify a more general cache.

            Many thanks
            Sam

            Show
            Sam Hemelryk added a comment - Thanks for looking fella's. Re: enrollment state + statuses, I was concerned that may be the case. I'd had a quick scan through the code and couldn't find anywhere those were being factored in with the icons but I figured they'd be somewhere. Not to mention I suppose we can't be sure that the plugins aren't delivering user specific icons as well I suppose. No probs, this was a quick knock up. I'll invest more time and look to identify a more general cache. Many thanks Sam
            Hide
            Petr Skoda added a comment -

            Thanks.

            Show
            Petr Skoda added a comment - Thanks.
            Hide
            Sam Hemelryk added a comment -

            Ok I've a much simpler cache now that simply caches the course enrolment instances that would otherwise be retrieved from the database.

            Show
            Sam Hemelryk added a comment - Ok I've a much simpler cache now that simply caches the course enrolment instances that would otherwise be retrieved from the database.
            Sam Hemelryk made changes -
            Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
            Pull Master Diff URL https://github.com/samhemelryk/moodle/compare/master...wip-MDL-29594-m25 https://github.com/samhemelryk/moodle/compare/master...wip-MDL-29594-m25-r2
            Pull Master Branch wip-MDL-29594-m25 wip-MDL-29594-m25-r2
            Hide
            Sam Hemelryk added a comment -

            Ready for peer-review once more

            Show
            Sam Hemelryk added a comment - Ready for peer-review once more
            Hide
            Petr Skoda added a comment -

            I am afraid this would not work because at present plugins modify the enrol table directly - see enrol/manual/edit.php for example. We would have to create new update_instance() method in enrol plugins and enforce its usage. I do not think we can do this change in stable branches because it might lead to regressions.

            I also do not understand the persistentmaxsize=2.

            The rest looks ok, I think there might be some more places that could use this. This might even allow me to add more flexibility later, I intentionally simplified some other code to prevent repeated fetching from database.

            Thanks for working on this.

            Show
            Petr Skoda added a comment - I am afraid this would not work because at present plugins modify the enrol table directly - see enrol/manual/edit.php for example. We would have to create new update_instance() method in enrol plugins and enforce its usage. I do not think we can do this change in stable branches because it might lead to regressions. I also do not understand the persistentmaxsize=2. The rest looks ok, I think there might be some more places that could use this. This might even allow me to add more flexibility later, I intentionally simplified some other code to prevent repeated fetching from database. Thanks for working on this.
            Hide
            Andrew Davis added a comment -

            This is not a proper review but simply a reminder to add some proper testing instructions.

            Show
            Andrew Davis added a comment - This is not a proper review but simply a reminder to add some proper testing instructions.
            Frédéric Massart made changes -
            Original Estimate 0 minutes [ 0 ]
            Remaining Estimate 0 minutes [ 0 ]
            Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
            Peer reviewer fred
            Hide
            Frédéric Massart added a comment -

            Putting back in development as it requires more work.

            Show
            Frédéric Massart added a comment - Putting back in development as it requires more work.
            Frédéric Massart made changes -
            Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
            Frédéric Massart made changes -
            Peer reviewer fred
            Sam Hemelryk made changes -
            Link This issue is blocked by MDL-45508 [ MDL-45508 ]
            Hide
            Sam Hemelryk added a comment -

            Linking to MDL-45508 which has been created to see an API created for enrol instance interaction.

            Show
            Sam Hemelryk added a comment - Linking to MDL-45508 which has been created to see an API created for enrol instance interaction.
            Sam Hemelryk made changes -
            Status Development in progress [ 3 ] Open [ 1 ]
            Sam Hemelryk made changes -
            Affects Version/s 2.7 [ 12850 ]
            Hide
            Sam Hemelryk added a comment -

            Stopping development of this until the blocker is resolved.

            Show
            Sam Hemelryk added a comment - Stopping development of this until the blocker is resolved.
            Hide
            Sam Hemelryk added a comment -

            I am un-assigning myself from this issue as I am not currently working on this and it will give the opportunity for someone else to work on it.

            Show
            Sam Hemelryk added a comment - I am un-assigning myself from this issue as I am not currently working on this and it will give the opportunity for someone else to work on it.
            Sam Hemelryk made changes -
            Fix Version/s BACKEND [ 12582 ]
            Fix Version/s STABLE backlog [ 10463 ]
            Labels triaged patch triaged
            Assignee Sam Hemelryk [ samhemelryk ]

              People

              • Votes:
                2 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated: