Moodle
  1. Moodle
  2. MDL-43721

It's VERY low performance of assign grading

    Details

    • Testing Instructions:
      Hide
      1. Create a course with 1000 students
      2. Enable enablegroupmembersonly experimental setting
      3. Use auto create groups to create 5 groups in a grouping
      4. Remove 4 of the groups from the grouping
      5. Create an assignment, with the grouping set to this new grouping and available for group members only checked.
      6. View the assignment summary and grading page.
      7. Verify they load pretty quick (< 3 secs) and only show about 1/5 of the students from the course.
      8. Edit the assignment settings. Uncheck the "available for group members only" setting
      9. View the assignment summary and grading page.
      10. Verify they load pretty quick (< 3 secs) and show all of the students from the course.
      Show
      Create a course with 1000 students Enable enablegroupmembersonly experimental setting Use auto create groups to create 5 groups in a grouping Remove 4 of the groups from the grouping Create an assignment, with the grouping set to this new grouping and available for group members only checked. View the assignment summary and grading page. Verify they load pretty quick (< 3 secs) and only show about 1/5 of the students from the course. Edit the assignment settings. Uncheck the "available for group members only" setting View the assignment summary and grading page. Verify they load pretty quick (< 3 secs) and show all of the students from the course.
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull 2.6 Branch:
    • Pull Master Branch:
      MDL-43721-master

      Description

      If there are 600 students in a course,when a teacher open a assign grading page url(like mod/assign/view.php?id=333&action=grading),he must waiting 20 seconds at least.
      envionment:

      1.600+ students in a course.
      2. rowsperpage: 50 or 100
      3.CPU on server masterfrequency: 2.0 G

      I traced the code,it shows the most time spent is on the line 1142 ( $this->build_table() ) in /lib/tablelib.php.

      so i wonder if it is possible to move this action on the side of browser via YUI? if the server side(PHP) only read the data from DB,and passing the meta data to the client,i think it will improve performance much more.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Chris Wharton added a comment - - edited

            Hi

            I am experiencing this problem too, with Moodle 2.6.1.
            Any page load of mod/assign/view.php?id=150814 takes a large amount of time, sometimes close to 60 seconds. There seems to be a lot more DB reads than necessary.
            I haven't backtraced the code to find the exact slow part yet.

            Example:
            mod/assign/view.php?id=150814&action=grading
            This course has 158 enrolled users.

            Assignments per page: 10
            Filter: none
            Quick grading: no
            Show only active enrolments: yes

            statistics:
            24.913944 secs
            RAM: 46MB
            RAM peak: 47.3MB
            Included 376 files
            Contexts for which filters were loaded: 1
            Filters created: 8
            Pieces of content filtered: 1
            Strings filtered: 0
            get_string calls: 526
            Included YUI modules: 0
            Other JavaScript modules: 3
            DB reads/writes: 1446/0
            ticks: 2491 user: 233 sys: 12 cuser: 115 csys: 21
            Load average: 1.57
            Session (core\session\memcache): 72.7KB
            This page is: General type: incourse. Context Assignment: Assignment (context id 245162). Page type mod-assign-view.
            

            Show
            Chris Wharton added a comment - - edited Hi I am experiencing this problem too, with Moodle 2.6.1. Any page load of mod/assign/view.php?id=150814 takes a large amount of time, sometimes close to 60 seconds. There seems to be a lot more DB reads than necessary. I haven't backtraced the code to find the exact slow part yet. Example: mod/assign/view.php?id=150814&action=grading This course has 158 enrolled users. Assignments per page: 10 Filter: none Quick grading: no Show only active enrolments: yes statistics: 24.913944 secs RAM: 46MB RAM peak: 47.3MB Included 376 files Contexts for which filters were loaded: 1 Filters created: 8 Pieces of content filtered: 1 Strings filtered: 0 get_string calls: 526 Included YUI modules: 0 Other JavaScript modules: 3 DB reads/writes: 1446/0 ticks: 2491 user: 233 sys: 12 cuser: 115 csys: 21 Load average: 1.57 Session (core\session\memcache): 72.7KB This page is: General type: incourse. Context Assignment: Assignment (context id 245162). Page type mod-assign-view.
            Hide
            Chris Wharton added a comment -

            I believe the slow down is coming from MDL-41315, commit https://github.com/moodle/moodle/commit/a0e59f04dd422f494f326f5e6ddfcc3b5ae611b1 .
            Reverting this commit makes for a massive speed increase. Page now loads consistently for me in about 3.5 seconds.
            I've done some profiling, and the loop calls groups_course_module_visible for every user.

            foreach ($users as $userid => $user) {
                        if (!groups_course_module_visible($cm, $userid)) { // this check is slow
                            unset($users[$userid]);
                        }
                    }
            

            After revert of above commit, with identical parameters to the above comment:

            3.21816 secs
            RAM: 38.3MB
            RAM peak: 39MB
            Included 339 files
            Contexts for which filters were loaded: 23
            Filters created: 184
            Pieces of content filtered: 1
            Strings filtered: 202
            get_string calls: 531
            Included YUI modules: 0
            Other JavaScript modules: 3
            DB reads/writes: 516/3
            ticks: 321 user: 124 sys: 33 cuser: 113 csys: 18
            Load average: 0.85
            Session (core\session\memcache): 31.5KB
            This page is: General type: incourse. Context Assignment: Assignment (context id 245162). Page type mod-assign-view.
            

            Show
            Chris Wharton added a comment - I believe the slow down is coming from MDL-41315 , commit https://github.com/moodle/moodle/commit/a0e59f04dd422f494f326f5e6ddfcc3b5ae611b1 . Reverting this commit makes for a massive speed increase. Page now loads consistently for me in about 3.5 seconds. I've done some profiling, and the loop calls groups_course_module_visible for every user. foreach ($users as $userid => $user) { if (!groups_course_module_visible($cm, $userid)) { // this check is slow unset($users[$userid]); } } After revert of above commit, with identical parameters to the above comment: 3.21816 secs RAM: 38.3MB RAM peak: 39MB Included 339 files Contexts for which filters were loaded: 23 Filters created: 184 Pieces of content filtered: 1 Strings filtered: 202 get_string calls: 531 Included YUI modules: 0 Other JavaScript modules: 3 DB reads/writes: 516/3 ticks: 321 user: 124 sys: 33 cuser: 113 csys: 18 Load average: 0.85 Session (core\session\memcache): 31.5KB This page is: General type: incourse. Context Assignment: Assignment (context id 245162). Page type mod-assign-view.
            Hide
            Chris Wharton added a comment - - edited

            I've attached a zip of two cachegrind traces generated with php-xdebug. Open with kcachegrind or similar.

            Show
            Chris Wharton added a comment - - edited I've attached a zip of two cachegrind traces generated with php-xdebug. Open with kcachegrind or similar.
            Hide
            Damyon Wiese added a comment -

            Thanks for chasing it down. It was known at the time that commit would make things slower, but more correct.

            Now we have a target for improvement, we could look at using caching or prefetching to speed up that check.

            Show
            Damyon Wiese added a comment - Thanks for chasing it down. It was known at the time that commit would make things slower, but more correct. Now we have a target for improvement, we could look at using caching or prefetching to speed up that check.
            Hide
            Tyron Delean added a comment -

            Hi Damyon,
            We were also experiencing this performance issue, but have made a custom workaround. I have included the changes we have implemented for your review. I would assume that the code change you may likely apply will be formatted differently than what I have provided, but the concept has worked fairly well for us.
            The performance issue obviously occurs due the fact that an iteration is required over every student after the initial enrolled users function is called. This is used to determine if the student has group membership for the course when groupmembersonly is selected, and additionally for a grouping if one is specified.
            What we have changed is to add the group members sql into the actual enrolled users query, to provide a single database hit to obtain this level of information.
            Please review the 2 text files attached.
            1) The changes to the list_participants function that exists in /mod/assign/locallib.php
            2) The changes to the get_enrolled_users function that exists in /lib/accesslib.php

            I hope that a similar change could be implemented to resolve some of these performance issues.
            Best Regards

            Show
            Tyron Delean added a comment - Hi Damyon, We were also experiencing this performance issue, but have made a custom workaround. I have included the changes we have implemented for your review. I would assume that the code change you may likely apply will be formatted differently than what I have provided, but the concept has worked fairly well for us. The performance issue obviously occurs due the fact that an iteration is required over every student after the initial enrolled users function is called. This is used to determine if the student has group membership for the course when groupmembersonly is selected, and additionally for a grouping if one is specified. What we have changed is to add the group members sql into the actual enrolled users query, to provide a single database hit to obtain this level of information. Please review the 2 text files attached. 1) The changes to the list_participants function that exists in /mod/assign/locallib.php 2) The changes to the get_enrolled_users function that exists in /lib/accesslib.php I hope that a similar change could be implemented to resolve some of these performance issues. Best Regards
            Hide
            Generazion Consulting S.L. added a comment -

            Hi, Tyron.

            We are experiencing the same performance issue too.

            However, after applying your patches to accesslib.php and locallib.php, we have not obtained any performance gain. For 196 participants we have execution times of 20-25 seconds.

            We tried to test the commit revert suggested by Chris without success. We obtain the following error:

            PHP Fatal error: Call to undefined method assign::count_participants() in /var/www/mod/assign/locallib.php on line 3892

            Any clues?

            Best Regards

            Show
            Generazion Consulting S.L. added a comment - Hi, Tyron. We are experiencing the same performance issue too. However, after applying your patches to accesslib.php and locallib.php, we have not obtained any performance gain. For 196 participants we have execution times of 20-25 seconds. We tried to test the commit revert suggested by Chris without success. We obtain the following error: PHP Fatal error: Call to undefined method assign::count_participants() in /var/www/mod/assign/locallib.php on line 3892 Any clues? Best Regards
            Hide
            Michael de Raadt added a comment -

            This has been discussed on the Partner forum, so I thought it should get some attention here in the Tracker.

            Show
            Michael de Raadt added a comment - This has been discussed on the Partner forum, so I thought it should get some attention here in the Tracker.
            Hide
            Michael de Raadt added a comment - - edited

            I just had an incidental report of this by email. I suspect this may be an issue looming to become bigger.

            Also noting that the case reported did not involve groupings.

            Show
            Michael de Raadt added a comment - - edited I just had an incidental report of this by email. I suspect this may be an issue looming to become bigger. Also noting that the case reported did not involve groupings.
            Hide
            Derick Turner added a comment -

            One of our clients is also experiencing this issue. Numbers students in the class are significantly less, however (~250) with the user typically experiencing 6 - 10 seconds of wait time whilst clicking on anything within the gradebook.

            Show
            Derick Turner added a comment - One of our clients is also experiencing this issue. Numbers students in the class are significantly less, however (~250) with the user typically experiencing 6 - 10 seconds of wait time whilst clicking on anything within the gradebook.
            Hide
            Chris Kenniburg added a comment - - edited

            We just updated last week and are having the same problem. It practically overwhelms our server when teachers go in to do grading. In our experience showing 100 users in Quickgrading of assignments results in 30-60 seconds to load. Most of the courses have groups. I have not been able to find a course without groups to test out.

            Show
            Chris Kenniburg added a comment - - edited We just updated last week and are having the same problem. It practically overwhelms our server when teachers go in to do grading. In our experience showing 100 users in Quickgrading of assignments results in 30-60 seconds to load. Most of the courses have groups. I have not been able to find a course without groups to test out.
            Hide
            Damyon Wiese added a comment -

            Done and ready for peer review.

            In my test course the results are:

            • Reduces DB queries from 6198/1 to 256/3.
            • Reduces time to show the page from 12secs down to 2.5secs.

            I changed 2 things.

            1 - added a function to grouplib to do this filtering of users in a list
            2 - added a static cache to mod_assign to prevent fetching the same user list twice in a request

            Show
            Damyon Wiese added a comment - Done and ready for peer review. In my test course the results are: Reduces DB queries from 6198/1 to 256/3. Reduces time to show the page from 12secs down to 2.5secs. I changed 2 things. 1 - added a function to grouplib to do this filtering of users in a list 2 - added a static cache to mod_assign to prevent fetching the same user list twice in a request
            Hide
            CiBoT added a comment -

            Results for MDL-43721

            • Remote repository: git://github.com/damyon/moodle.git
            Show
            CiBoT added a comment - Results for MDL-43721 Remote repository: git://github.com/damyon/moodle.git Remote branch MDL-43721 -25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1773 Warning: The MDL-43721 -25 branch at git://github.com/damyon/moodle.git has not been rebased recently (>20 days ago). Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1773/artifact/work/smurf.html Remote branch MDL-43721 -26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1774 Warning: The MDL-43721 -26 branch at git://github.com/damyon/moodle.git has not been rebased recently (>20 days ago). Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1774/artifact/work/smurf.html Remote branch MDL-43721 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1775 Error: The MDL-43721 -master branch at git://github.com/damyon/moodle.git exceeds the maximum number of commits ( 53 > 15)
            Hide
            Damyon Wiese added a comment -

            This is failing unit tests - I must have borked something.

            Show
            Damyon Wiese added a comment - This is failing unit tests - I must have borked something.
            Hide
            Damyon Wiese added a comment -

            Found my bug and this is passing unit tests now. The unit tests in mod_assign "test_group_members_only()" are specifically testing this new function from grouplib - I hope that's enough.

            Show
            Damyon Wiese added a comment - Found my bug and this is passing unit tests now. The unit tests in mod_assign "test_group_members_only()" are specifically testing this new function from grouplib - I hope that's enough.
            Hide
            CiBoT added a comment -

            Results for MDL-43721

            • Remote repository: git://github.com/damyon/moodle.git
            Show
            CiBoT added a comment - Results for MDL-43721 Remote repository: git://github.com/damyon/moodle.git Remote branch MDL-43721 -25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1796 Warning: The MDL-43721 -25 branch at git://github.com/damyon/moodle.git has not been rebased recently (>20 days ago). Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1796/artifact/work/smurf.html Remote branch MDL-43721 -26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1797 Warning: The MDL-43721 -26 branch at git://github.com/damyon/moodle.git has not been rebased recently (>20 days ago). Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1797/artifact/work/smurf.html Remote branch MDL-43721 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1798 Error: The MDL-43721 -master branch at git://github.com/damyon/moodle.git exceeds the maximum number of commits ( 53 > 15)
            Hide
            Shuming Liu added a comment -

            i think it's nothing to do with the performance of reading or writting db.

            because when i tracing the grading code in my development envionment,it only spent less than 20 secs.In the development envionment,i connect the same mysql db server,but my developing computer's cpu is faster than the web server's.

            Show
            Shuming Liu added a comment - i think it's nothing to do with the performance of reading or writting db. because when i tracing the grading code in my development envionment,it only spent less than 20 secs.In the development envionment,i connect the same mysql db server,but my developing computer's cpu is faster than the web server's.
            Hide
            Damyon Wiese added a comment -

            Shuming Liu

            The speed is to do with the number of individual queries. Without this patch - because of the group api the assignment is making 4-5 DB calls per student - and doing this multiple times in the one page load. With this patch - it's 3 db calls regardless of the number of students (just for the problem bit of code).

            Show
            Damyon Wiese added a comment - Shuming Liu The speed is to do with the number of individual queries. Without this patch - because of the group api the assignment is making 4-5 DB calls per student - and doing this multiple times in the one page load. With this patch - it's 3 db calls regardless of the number of students (just for the problem bit of code).
            Hide
            Shuming Liu added a comment -

            To load exactly the same grading page,my developing envionment spent only 1/4 to 1/3 of the time in my web server,so i think the DB calls is not the most important factor.the most time spent is on the line 1142 ( $this->build_table() ) in /lib/tablelib.php.when i commented this line,page will load more quickly.

            Show
            Shuming Liu added a comment - To load exactly the same grading page,my developing envionment spent only 1/4 to 1/3 of the time in my web server,so i think the DB calls is not the most important factor.the most time spent is on the line 1142 ( $this->build_table() ) in /lib/tablelib.php.when i commented this line,page will load more quickly.
            Hide
            Chung-liang, Wei added a comment - - edited

            After tracing the code, I think it is the function 'other_cols()' in /mod/assign/gradingtable.php that causes the problem.
            It will repeatedly call $plugin->is_visible() and $plugin->is_enabled() for a plugin, while these two functions perform DB calls to determine the configuration of the specified plugin.
            If we can cache the result of these two functions after they were called for the first time, the unnecessary DB calls checking the status of the same plugin can be bypassed.

            -----
            --- gradingtable.php    2014-02-28 10:02:43.000000000 +0800
            +++ gradingtable.php.new        2014-03-05 15:43:02.128697285 +0800
            @@ -1137,8 +1137,12 @@
                     if (isset($plugincache[1])) {
                         $field = $plugincache[1];
                     }
            -
            -        if ($plugin->is_visible() && $plugin->is_enabled()) {
            +               static $cache_visible = array(), $cache_enabled = array();
            +               if (!isset($cache_visible[$colname])) {
            +                       $cache_visible[$colname] = $plugin->is_visible();
            +                       $cache_enabled[$colname] = $plugin->is_enabled();
            +               }
            +        if ($cache_visible[$colname] && $cache_enabled[$colname]) {
                         if ($plugin->get_subtype() == 'assignsubmission') {
                             if ($this->assignment->get_instance()->teamsubmission) {
                                 $group = false;
            -----
            

            Show
            Chung-liang, Wei added a comment - - edited After tracing the code, I think it is the function 'other_cols()' in /mod/assign/gradingtable.php that causes the problem. It will repeatedly call $plugin->is_visible() and $plugin->is_enabled() for a plugin, while these two functions perform DB calls to determine the configuration of the specified plugin. If we can cache the result of these two functions after they were called for the first time, the unnecessary DB calls checking the status of the same plugin can be bypassed. ----- --- gradingtable.php 2014-02-28 10:02:43.000000000 +0800 +++ gradingtable.php.new 2014-03-05 15:43:02.128697285 +0800 @@ -1137,8 +1137,12 @@ if (isset($plugincache[1])) { $field = $plugincache[1]; } - - if ($plugin->is_visible() && $plugin->is_enabled()) { + static $cache_visible = array(), $cache_enabled = array(); + if (!isset($cache_visible[$colname])) { + $cache_visible[$colname] = $plugin->is_visible(); + $cache_enabled[$colname] = $plugin->is_enabled(); + } + if ($cache_visible[$colname] && $cache_enabled[$colname]) { if ($plugin->get_subtype() == 'assignsubmission') { if ($this->assignment->get_instance()->teamsubmission) { $group = false; -----
            Hide
            Shuming Liu added a comment -

            Chung-liang, Wei
            thanks for your code,it really works well,i changed the code you contributed,it only takes 4 secs to load the grading page now.

            Show
            Shuming Liu added a comment - Chung-liang, Wei thanks for your code,it really works well,i changed the code you contributed,it only takes 4 secs to load the grading page now.
            Hide
            Damyon Wiese added a comment -

            Thanks Chung-liang, Wei

            I believed that those calls from get_config should have come from cache - but testing shows that they don't.

            I added a commit to all branches that adds a cache for these because they are called so often. I put the cache inside the assignment_plugin class - using static variables for caches leads to problems with unit tests (because the caches don't get reset).

            On a page showing 100 users with all plugins enabled, this reduces the db calls by about 200 for me.

            Show
            Damyon Wiese added a comment - Thanks Chung-liang, Wei I believed that those calls from get_config should have come from cache - but testing shows that they don't. I added a commit to all branches that adds a cache for these because they are called so often. I put the cache inside the assignment_plugin class - using static variables for caches leads to problems with unit tests (because the caches don't get reset). On a page showing 100 users with all plugins enabled, this reduces the db calls by about 200 for me.
            Hide
            Damyon Wiese added a comment -

            (Unit tests passed on all branches).

            Show
            Damyon Wiese added a comment - (Unit tests passed on all branches).
            Hide
            Frédéric Massart added a comment -

            Hi Damyon,

            thanks for fixing that. I just have a few thoughts:

            1. You seem to be doing a lot of array_* calls in here. PHP is not proven to be very efficient when it comes to array manipulation. Do you think you could reduce them a little bit?
              • I am particularly frightened by: array_intersect_key($users, array_flip(array_merge($aag, $ingroup)));
            2. I think it'd be easier to read through the new function in groups if the variables were more self explained. Prefer $accessallx instead of $aax. Also the call to get_enrolled_sql() could be placed right before its return values are used.
            3. show_only_active_users() returns a boolean, perhaps using a string for the $key would be safer.
            4. In general I think it's best not to use static variables, especially for Unit Tests. Have you thought of using an MUC request cache?
            5. For the SQL queries, why not using a JOIN? http://stackoverflow.com/questions/894490/sql-left-join-vs-multiple-tables-on-from-line
            6. Also, I tend to prefer named arguments rather than the index-based ones. Especially when using array_merge() as I don't have faith in PHP keeping the right order/keys...

            I have not tested the patch myself, but I believe you have successfully identified the source of problem here.

            Cheers,
            Fred

            Show
            Frédéric Massart added a comment - Hi Damyon, thanks for fixing that. I just have a few thoughts: You seem to be doing a lot of array_* calls in here. PHP is not proven to be very efficient when it comes to array manipulation. Do you think you could reduce them a little bit? I am particularly frightened by: array_intersect_key($users, array_flip(array_merge($aag, $ingroup))); I think it'd be easier to read through the new function in groups if the variables were more self explained. Prefer $accessallx instead of $aax. Also the call to get_enrolled_sql() could be placed right before its return values are used. show_only_active_users() returns a boolean, perhaps using a string for the $key would be safer. In general I think it's best not to use static variables, especially for Unit Tests. Have you thought of using an MUC request cache? For the SQL queries, why not using a JOIN? http://stackoverflow.com/questions/894490/sql-left-join-vs-multiple-tables-on-from-line Also, I tend to prefer named arguments rather than the index-based ones. Especially when using array_merge() as I don't have faith in PHP keeping the right order/keys... I have not tested the patch myself, but I believe you have successfully identified the source of problem here. Cheers, Fred
            Hide
            Damyon Wiese added a comment -

            Thanks Fred,

            I have updated the patch based on your review:

            1. No - they are useful - I added a comment to explain what they are doing.
            2. Renamed and reordered
            3. It is concatenated to the end of the string - so it will always be cast to a string.
            4. These aren't static variables, they are (private) class instance variables. Php unit tests never share the same instance of a class, so the cache gets automatically cleared.
            5. Changed to a join
            6. Changed to named arguments

            Sending for integration - thanks!

            Show
            Damyon Wiese added a comment - Thanks Fred, I have updated the patch based on your review: 1. No - they are useful - I added a comment to explain what they are doing. 2. Renamed and reordered 3. It is concatenated to the end of the string - so it will always be cast to a string. 4. These aren't static variables, they are (private) class instance variables. Php unit tests never share the same instance of a class, so the cache gets automatically cleared. 5. Changed to a join 6. Changed to named arguments Sending for integration - thanks!
            Hide
            Generazion Consulting S.L. added a comment -

            Thank very much, Chung-liang, Wei. We patched gradingtable.php and grading page load passed from 30s to 5-6 seconds!.

            Daymon, we tried your 27/Feb/14 patch and we didn't notice performance improvements in our lab tests. Do you think it could be combined with Chung-liang, Wei patch? Are you preparing a combined patch?

            Thank you very much for your help and support,
            Best Regards

            Show
            Generazion Consulting S.L. added a comment - Thank very much, Chung-liang, Wei. We patched gradingtable.php and grading page load passed from 30s to 5-6 seconds!. Daymon, we tried your 27/Feb/14 patch and we didn't notice performance improvements in our lab tests. Do you think it could be combined with Chung-liang, Wei patch? Are you preparing a combined patch? Thank you very much for your help and support, Best Regards
            Hide
            Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            Damyon Wiese added a comment -

            The current branches on this issue include both improvements (I updated them yesterday). I put the caching in a different spot but it will have the same effect.

            Show
            Damyon Wiese added a comment - The current branches on this issue include both improvements (I updated them yesterday). I put the caching in a different spot but it will have the same effect.
            Hide
            Chung-liang, Wei added a comment -

            Damyon Wiese's latest patch is better than my dirty hack.
            Waiting for the official release ...

            Show
            Chung-liang, Wei added a comment - Damyon Wiese's latest patch is better than my dirty hack. Waiting for the official release ...
            Hide
            CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            Sam Hemelryk added a comment -

            Thanks Damyon - this has been integrated now.

            Just noting that if I read correctly you could avoid the need for array_flip by not calling array_keys on the resulting data for $usersingroup and $userswithaccessallgroups.
            The data per item in those appears small and it would save you three array calls (perhaps easier to read code).
            Either way what you've got is a perfect anyway so no probs and in it goes.

            Show
            Sam Hemelryk added a comment - Thanks Damyon - this has been integrated now. Just noting that if I read correctly you could avoid the need for array_flip by not calling array_keys on the resulting data for $usersingroup and $userswithaccessallgroups. The data per item in those appears small and it would save you three array calls (perhaps easier to read code). Either way what you've got is a perfect anyway so no probs and in it goes.
            Hide
            Marina Glancy added a comment -

            Results on master (grading page):
            10 assignments per page:
            1.58041 secs
            DB reads/writes: 151/0

            100 assignments per page:
            10.621202 secs
            DB reads/writes: 961/0

            Is this ok?

            Show
            Marina Glancy added a comment - Results on master (grading page): 10 assignments per page: 1.58041 secs DB reads/writes: 151/0 100 assignments per page: 10.621202 secs DB reads/writes: 961/0 Is this ok?
            Hide
            Marina Glancy added a comment -

            2.6 (10pp): 1.745608 secs; DB reads/writes: 88/0
            2.6 (100pp): 10.42723 secs; DB reads/writes: 358/0

            2.5 (10pp): 0.309824 secs; DB reads/writes: 92/0
            2.5 (100pp): 0.537979 secs; DB reads/writes: 362/0

            looks like a lot of potential performance improvements in assignment.

            Show
            Marina Glancy added a comment - 2.6 (10pp): 1.745608 secs; DB reads/writes: 88/0 2.6 (100pp): 10.42723 secs; DB reads/writes: 358/0 2.5 (10pp): 0.309824 secs; DB reads/writes: 92/0 2.5 (100pp): 0.537979 secs; DB reads/writes: 362/0 looks like a lot of potential performance improvements in assignment.
            Hide
            Marina Glancy added a comment -

            P.S. using enablegroupmembersonly and grouping does not affect time and number of db queries much

            Show
            Marina Glancy added a comment - P.S. using enablegroupmembersonly and grouping does not affect time and number of db queries much
            Hide
            Damyon Wiese added a comment -

            yes - this patch improves 2 specific things only. Alot of things are not really fixable without moving to a new interface that does not show every possible thing ever done in the assignment in a huge grid.

            revert the patch and you will see > 1200 db reads.

            Show
            Damyon Wiese added a comment - yes - this patch improves 2 specific things only. Alot of things are not really fixable without moving to a new interface that does not show every possible thing ever done in the assignment in a huge grid. revert the patch and you will see > 1200 db reads.
            Hide
            Damyon Wiese added a comment -

            also try "enablegroupmembersonly and grouping" without this patch.

            Show
            Damyon Wiese added a comment - also try "enablegroupmembersonly and grouping" without this patch.
            Hide
            Marina Glancy added a comment -

            ok, passing. But still something needs to be done about 3 times more queries in 2.7 comparing to 2.6

            Show
            Marina Glancy added a comment - ok, passing. But still something needs to be done about 3 times more queries in 2.7 comparing to 2.6
            Hide
            Damyon Wiese added a comment -

            Created MDL-44583 to look at the difference in 2.7.

            Show
            Damyon Wiese added a comment - Created MDL-44583 to look at the difference in 2.7.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            The price of success is hard work,
            dedication to the job at hand,
            and the determination that whether we win or lose,
            we have applied the best of ourselves to the task at hand.

            Vince Lombardi

            This is now part of Moodle, your favorite non-frameworkial LMS, LOL. Thanks, closing!

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - The price of success is hard work, dedication to the job at hand, and the determination that whether we win or lose, we have applied the best of ourselves to the task at hand. Vince Lombardi This is now part of Moodle, your favorite non-frameworkial LMS, LOL. Thanks, closing! Ciao
            Hide
            Sam Marshall added a comment - - edited

            Drat! This change is causing a problem for me regarding MDL-44070.

            The problem is, as part of MDL-44070 we are intending to REMOVE groupmembersonly. Group and grouping access restriction will be treated the same way as every other access restriction (date, user profile, completion, grade) using a new API which allows for OR conditions and other things.

            As a result there is no longer an easy way to filter things using direct SQL queries as you are doing, which consider only the group condition.

            So after MDL-44070 there is now no way to check if somebody can access it, other than the time-consuming checks which can only really run for an individual. (Which is what causes the performance issue that you saw in the first place. And by the way this was also necessary to do it 'properly' before as if you don't have access for some other reason than groups, this code will still show them.)

            There are really a few ways to resolve this:

            1) Change this back to use the code I previously made it use. But this will have the performance problem again.

            2) Change it to ignore groupmembersonly and show/hide people based on the group mode and grouping selected. (In other words, if it is set to visible groups, do not hide anybody; if it's separate groups, include only people who are in that grouping.) Basically this is the same code you wrote already, but using the activity groups flag instead of groupmembersonly.

            3) Do some horrible hack within my new code that lets me implement a function like yours that somehow only takes into account the group/grouping restrictions and not any others.

            I think 2 is the best option but the disadvantage is that behaviour may slightly change (i.e. if you previously used separate groups mode and did NOT have groupmembersonly, previously it showed even users who were not in groups, but after this change it would not do so).

            I'll code this as part of my new development but Damyon, please could you let me know what you think? Thanks.

            In case it wasn't clear - I've made a commit already that does option 2. Currently it is in my branch but we could do it as a separate issue beforehand if you are happy with it (my branch is massive enough already).

            (edit: first commit was wrong, this one might be better)
            https://github.com/sammarshallou/moodle/commit/e3f1834174d9b22a5f2ca7a625cf42b7ad35cc6b

            Show
            Sam Marshall added a comment - - edited Drat! This change is causing a problem for me regarding MDL-44070 . The problem is, as part of MDL-44070 we are intending to REMOVE groupmembersonly. Group and grouping access restriction will be treated the same way as every other access restriction (date, user profile, completion, grade) using a new API which allows for OR conditions and other things. As a result there is no longer an easy way to filter things using direct SQL queries as you are doing, which consider only the group condition. So after MDL-44070 there is now no way to check if somebody can access it, other than the time-consuming checks which can only really run for an individual. (Which is what causes the performance issue that you saw in the first place. And by the way this was also necessary to do it 'properly' before as if you don't have access for some other reason than groups, this code will still show them.) There are really a few ways to resolve this: 1) Change this back to use the code I previously made it use. But this will have the performance problem again. 2) Change it to ignore groupmembersonly and show/hide people based on the group mode and grouping selected. (In other words, if it is set to visible groups, do not hide anybody; if it's separate groups, include only people who are in that grouping.) Basically this is the same code you wrote already, but using the activity groups flag instead of groupmembersonly. 3) Do some horrible hack within my new code that lets me implement a function like yours that somehow only takes into account the group/grouping restrictions and not any others. I think 2 is the best option but the disadvantage is that behaviour may slightly change (i.e. if you previously used separate groups mode and did NOT have groupmembersonly, previously it showed even users who were not in groups, but after this change it would not do so). I'll code this as part of my new development but Damyon, please could you let me know what you think? Thanks. In case it wasn't clear - I've made a commit already that does option 2. Currently it is in my branch but we could do it as a separate issue beforehand if you are happy with it (my branch is massive enough already). (edit: first commit was wrong, this one might be better) https://github.com/sammarshallou/moodle/commit/e3f1834174d9b22a5f2ca7a625cf42b7ad35cc6b
            Hide
            Damyon Wiese added a comment -

            Hi Sam - sorry for conflicting with your condition work - but in this case - this is a valid thing we are trying to do and we need a solution that lets us do it quickly.

            I would like to see:

            option 4. Add a new magic function that takes a list of users and a coursemodule and returns only the users who can see the course module (just like I did for groups here).

            This is really a requirement IMO.

            About your commit - I haven't seen the whole patch - but my new function has been added now - and to remove it you will need to deprecate it properly (the deprecated version could call the new version from conditionlib).

            Show
            Damyon Wiese added a comment - Hi Sam - sorry for conflicting with your condition work - but in this case - this is a valid thing we are trying to do and we need a solution that lets us do it quickly. I would like to see: option 4. Add a new magic function that takes a list of users and a coursemodule and returns only the users who can see the course module (just like I did for groups here). This is really a requirement IMO. About your commit - I haven't seen the whole patch - but my new function has been added now - and to remove it you will need to deprecate it properly (the deprecated version could call the new version from conditionlib).
            Hide
            Sam Marshall added a comment -

            OK I am creating a new issue for this. I disagree with your assessment, but I'll say why in there. MDL-44692. Still working on that, will put a patch in there.

            By the way I definitely agree that the performance problem needed to be fixed, I just feel that we should not attempt to exactly determine which users can access the activity as this is infeasible within current (2.5, 2.6, 2.7) Moodle.

            Show
            Sam Marshall added a comment - OK I am creating a new issue for this. I disagree with your assessment, but I'll say why in there. MDL-44692 . Still working on that, will put a patch in there. By the way I definitely agree that the performance problem needed to be fixed, I just feel that we should not attempt to exactly determine which users can access the activity as this is infeasible within current (2.5, 2.6, 2.7) Moodle.
            Hide
            Damien Bezborodov added a comment -

            I have an unrelated performance fix for assignment grading table in MDL-45678 that is currently pending peer review and testing.

            Show
            Damien Bezborodov added a comment - I have an unrelated performance fix for assignment grading table in MDL-45678 that is currently pending peer review and testing.

              People

              • Votes:
                13 Vote for this issue
                Watchers:
                23 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: