Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-34223

forum_get_file_info() performance improvement

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1, 2.2, 2.3
    • Fix Version/s: 2.3.3
    • Component/s: Forum
    • Labels:

      Description

      while working on MDL-33857 I noticed that forum_get_file_info() is inefficient especially when being run multiple times. This happens on moodle.org with a lot of forums and user groups when admin/teacher browses the Server files repositories

        Gliffy Diagrams

          Activity

          Hide
          marina Marina Glancy added a comment -

          TO INTEGRATORS: please cherry-pick also to 2.3

          This change basically avoids calling groups_is_member() if user has permission to view any group. This decreased time of building Server files list (on the biggest forum) from 11 down to 8 seconds. Which is of course also a lot, and MDL-33857 needs to deal with it.

          But anyway this is too much for one function. I noticed that table groups_members does not have an index on (userid, groupid).

          Show
          marina Marina Glancy added a comment - TO INTEGRATORS: please cherry-pick also to 2.3 This change basically avoids calling groups_is_member() if user has permission to view any group. This decreased time of building Server files list (on the biggest forum) from 11 down to 8 seconds. Which is of course also a lot, and MDL-33857 needs to deal with it. But anyway this is too much for one function. I noticed that table groups_members does not have an index on (userid, groupid).
          Hide
          marina Marina Glancy added a comment -

          I created a branch for 2.3 as well.

          Show
          marina Marina Glancy added a comment - I created a branch for 2.3 as well.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

          Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks Marina, this has been integrated now.

          I'm not a big fan of static vars like this but given MUC should be arriving shortly (fingers crossed) and the forum is likely to be replaced/widely addressed in the near future I think this is fine.
          Hopefully in the new forum we can look at ways to handle the caching of those areas post - discussion - forum at a lower level.

          Many thanks
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks Marina, this has been integrated now. I'm not a big fan of static vars like this but given MUC should be arriving shortly (fingers crossed) and the forum is likely to be replaced/widely addressed in the near future I think this is fine. Hopefully in the new forum we can look at ways to handle the caching of those areas post - discussion - forum at a lower level. Many thanks Sam
          Hide
          fred Frédéric Massart added a comment -

          Test successful on 2.3 and master. Cheers!

          Show
          fred Frédéric Massart added a comment - Test successful on 2.3 and master. Cheers!
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Gutta cavat lapidem, non vi sed saepe cadendo - Ovidio

          This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

          Thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Gutta cavat lapidem, non vi sed saepe cadendo - Ovidio This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks!

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                12/Nov/12