Moodle
  1. Moodle
  2. MDL-34223

forum_get_file_info() performance improvement

    Details

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

      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

        Activity

        Hide
        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 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 Glancy added a comment -

        I created a branch for 2.3 as well.

        Show
        Marina Glancy added a comment - I created a branch for 2.3 as well.
        Hide
        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
        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
        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
        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
        Frédéric Massart added a comment -

        Test successful on 2.3 and master. Cheers!

        Show
        Frédéric Massart added a comment - Test successful on 2.3 and master. Cheers!
        Hide
        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
        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: