Moodle
  1. Moodle
  2. MDL-29594

Course category tree on frontpage costs a db query per course

    Details

    • Type: Bug Bug
    • Status: Development in progress
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.1.1
    • Fix Version/s: STABLE backlog
    • Component/s: Performance
    • Labels:
    • Rank:
      19108

      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

        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 Škoda 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 Škoda 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 Škoda added a comment -

          Thanks.

          Show
          Petr Škoda 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 Škoda 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 Škoda 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

            People

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

              Dates

              • Created:
                Updated: