Moodle
  1. Moodle
  2. MDL-45678

assignsubmission_comments: Improve performance for permissions checking

    Details

      Description

      When viewing the grading table, a permissions callback will check for suspended users for each submission in the table.

      Please view the attached callgraph alongside my explanation regarding my changes.

      In assign_submission_comments::view_summary(), there is a call to the comment constructor. This constructor calls comment::check_permissions(), which is expensive – for each row!

      According to the callgraph, the reason why comment::check_permissions() is expensive, is not because it calls has_capability(), but rather because it calls assignsubmission_comments_comment_permissions().

      If you view assignsubmission_comments_comment_permissions(), it checks get_suspended_userids(), which generates an expensive SQL query in pg_query_params().

      My fix is to cache the result of get_suspended_userids() in a MODE_REQUEST Cache.

        Gliffy Diagrams

        1. callgraph.svg
          231 kB
          Damien Bezborodov
        2. callgrind.out
          566 kB
          Damien Bezborodov
        1. callgraph.png
          4.38 MB
        2. grading-screen.png
          213 kB
        3. grading-screen-master27.png
          170 kB

          Issue Links

            Activity

            Show
            CiBoT added a comment - Results for MDL-45678 Remote repository: https://github.com/dbezborodovrp/moodle.git Remote branch MDL-45678 -26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3557 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3557/artifact/work/smurf.html Remote branch MDL-45678 -27 to be integrated into upstream MOODLE_27_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3558 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3558/artifact/work/smurf.html Remote branch MDL-45678 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3559 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3559/artifact/work/smurf.html
            Hide
            Damyon Wiese added a comment -

            Sorry - but this change sounds like it introduces a security hole. Just because the comments API is being called from a particular page - does not mean it can't be called from any other page that is not doing the suspended users check. Also the ajax comment script could be called directly and can't assume anything about the permissions that have been checked from the calling page.

            I haven't really checked the code in the patch - just the description of the issue.

            If you have reason to believe that this comment is incorrect and that this does not introduce a security hole - please argue the point.

            Thanks!

            Show
            Damyon Wiese added a comment - Sorry - but this change sounds like it introduces a security hole. Just because the comments API is being called from a particular page - does not mean it can't be called from any other page that is not doing the suspended users check. Also the ajax comment script could be called directly and can't assume anything about the permissions that have been checked from the calling page. I haven't really checked the code in the patch - just the description of the issue. If you have reason to believe that this comment is incorrect and that this does not introduce a security hole - please argue the point. Thanks!
            Hide
            Damien Bezborodov added a comment -

            > Just because the comments API is being called from a particular page

            I only override the permissions check on the page that I am calling it from. If I do not pass my custom permissions into the constructor, then it defaults to the current behaviour.

            Show
            Damien Bezborodov added a comment - > Just because the comments API is being called from a particular page I only override the permissions check on the page that I am calling it from. If I do not pass my custom permissions into the constructor, then it defaults to the current behaviour.
            Hide
            Damien Bezborodov added a comment - - edited

            > Also the ajax comment script could be called directly

            There are no changes to the AJAX script at all. My change merely avoids calling assignsubmission_comments_comment_permissions() from the grading table.

            Thanks for having a look, Damyon.

            Show
            Damien Bezborodov added a comment - - edited > Also the ajax comment script could be called directly There are no changes to the AJAX script at all. My change merely avoids calling assignsubmission_comments_comment_permissions() from the grading table. Thanks for having a look, Damyon.
            Hide
            Damien Bezborodov added a comment - - edited

            Hi Damyon,

            I have just added some comments to Github to explain my changes.

            Please see https://github.com/dbezborodovrp/moodle/commit/2d63dbf#diff-4bdf5685507d22317715bb9204ec9d3eR308

            Show
            Damien Bezborodov added a comment - - edited Hi Damyon, I have just added some comments to Github to explain my changes. Please see https://github.com/dbezborodovrp/moodle/commit/2d63dbf#diff-4bdf5685507d22317715bb9204ec9d3eR308
            Hide
            Marina Glancy added a comment -

            Damien, I also think that this approach is not correct. First of all it, this logic is difficult to understand - why passing capabilities in options avoids calling callback?

            The best solution would be to implement some logic inside the callback itself that improves performance. If for some reason it is not possible, just pass something like $options->avoidcallback = true. But in this case you really need to convince us that there is no other way to implement it inside the callback.

            Show
            Marina Glancy added a comment - Damien, I also think that this approach is not correct. First of all it, this logic is difficult to understand - why passing capabilities in options avoids calling callback? The best solution would be to implement some logic inside the callback itself that improves performance. If for some reason it is not possible, just pass something like $options->avoidcallback = true. But in this case you really need to convince us that there is no other way to implement it inside the callback.
            Hide
            Damien Bezborodov added a comment -

            This seems to have been introduced in MDL-40218.

            Show
            Damien Bezborodov added a comment - This seems to have been introduced in MDL-40218 .
            Hide
            Damien Bezborodov added a comment - - edited

            I have requested peer review with feedback from Marina implemented on MDL-45678-002-master. Old changes are on MDL-45678-master

            I do not want to optimise the callback itself, since it may impact Rajesh's changes in MDL-40218.

            The callgraph attached to this ticket reveals that multiple calls to get_suspended_userids() for each row in the gradingtable's HTML table is what is causing this issue.

            Show
            Damien Bezborodov added a comment - - edited I have requested peer review with feedback from Marina implemented on MDL-45678-002-master . Old changes are on MDL-45678-master I do not want to optimise the callback itself, since it may impact Rajesh's changes in MDL-40218 . The callgraph attached to this ticket reveals that multiple calls to get_suspended_userids() for each row in the gradingtable's HTML table is what is causing this issue.
            Hide
            CiBoT added a comment -
            Show
            CiBoT added a comment - Results for MDL-45678 Remote repository: https://github.com/dbezborodovrp/moodle.git Remote branch MDL-45678 -002-master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3954 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3954/artifact/work/smurf.html
            Hide
            Marina Glancy added a comment -

            Thanks Damien, it definitely looks better than before.
            But still there should be reasonable explanation why not implement this inside callback. Asking Rajesh Taneja for second opinion.

            Show
            Marina Glancy added a comment - Thanks Damien, it definitely looks better than before. But still there should be reasonable explanation why not implement this inside callback. Asking Rajesh Taneja for second opinion.
            Hide
            Damien Bezborodov added a comment - - edited

            Yeah, I agree as I think about it a bit more. get_suspended_userids() should ideally be replaced with something more efficient... something that doesn't fetch the entire database table just to check a single user id. Something like is_enrolled() might work but I need to write a unit test to prove that the functionality is the same between these two functions.

            http://docs.moodle.org/dev/Enrolment_API#is_enrolled.28.29

            Show
            Damien Bezborodov added a comment - - edited Yeah, I agree as I think about it a bit more. get_suspended_userids() should ideally be replaced with something more efficient... something that doesn't fetch the entire database table just to check a single user id. Something like is_enrolled() might work but I need to write a unit test to prove that the functionality is the same between these two functions. http://docs.moodle.org/dev/Enrolment_API#is_enrolled.28.29
            Hide
            Damien Bezborodov added a comment -

            My latest changes are on MDL-45678-003-master

            There is a unit test under mod_assign_locallib_testcase::test_can_view_submission that proves functionality is not broken.

            Show
            Damien Bezborodov added a comment - My latest changes are on MDL-45678-003-master There is a unit test under mod_assign_locallib_testcase::test_can_view_submission that proves functionality is not broken.
            Show
            CiBoT added a comment - Results for MDL-45678 Remote repository: https://github.com/dbezborodovrp/moodle.git Remote branch MDL-45678 -003-26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3962 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3962/artifact/work/smurf.html Remote branch MDL-45678 -003-27 to be integrated into upstream MOODLE_27_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3963 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3963/artifact/work/smurf.html Remote branch MDL-45678 -003-master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3964 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3964/artifact/work/smurf.html
            Hide
            Marina Glancy added a comment -

            Damien, after this change the performance of the page decreases quite significatnly.

            Show
            Marina Glancy added a comment - Damien, after this change the performance of the page decreases quite significatnly.
            Hide
            Damyon Wiese added a comment -

            In the case for the grading table, the current code loads the list of suspended users and caches it because it is called for each student in the table. The comments case is different, because it's in a callback and each instance of the callback creates a new assign class - so the static cache is not hit.

            One solution would be to move the cache to an MUC static cache which would persist between instances of the assignment class.

            Show
            Damyon Wiese added a comment - In the case for the grading table, the current code loads the list of suspended users and caches it because it is called for each student in the table. The comments case is different, because it's in a callback and each instance of the callback creates a new assign class - so the static cache is not hit. One solution would be to move the cache to an MUC static cache which would persist between instances of the assignment class.
            Hide
            Damien Bezborodov added a comment -

            Sounds like a plan. I actually considered using a static variable, but decided that would be ugly and might upset unit tests with global state. I'll check out the Moodle Universal Cache to see if it will be suitable.

            It is kind of annoying since the table already excludes rows where a user account is suspended, so the callback does more work than it needs to, but other areas that use the callback could rely on it.

            Cheers, Damyon. Much appreciated.

            Show
            Damien Bezborodov added a comment - Sounds like a plan. I actually considered using a static variable, but decided that would be ugly and might upset unit tests with global state. I'll check out the Moodle Universal Cache to see if it will be suitable. It is kind of annoying since the table already excludes rows where a user account is suspended, so the callback does more work than it needs to, but other areas that use the callback could rely on it. Cheers, Damyon. Much appreciated.
            Hide
            Damien Bezborodov added a comment - - edited

            Page Request Cache implemented. I have never used the cache feature before, so could do with some feedback if appropriate.

            Tested locally based on an backup/restored course from a University's production Moodle instance with over 100 users. Page load time was reduced from 12 seconds to 6 seconds. It would probably be more significant for a site with many enrolled users, but I only tested this locally.

            Show
            Damien Bezborodov added a comment - - edited Page Request Cache implemented. I have never used the cache feature before, so could do with some feedback if appropriate. Tested locally based on an backup/restored course from a University's production Moodle instance with over 100 users. Page load time was reduced from 12 seconds to 6 seconds. It would probably be more significant for a site with many enrolled users, but I only tested this locally.
            Show
            CiBoT added a comment - Results for MDL-45678 Remote repository: https://github.com/dbezborodovrp/moodle.git Remote branch MDL-45678 -004-26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/4314 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/4314/artifact/work/smurf.html Remote branch MDL-45678 -004-27 to be integrated into upstream MOODLE_27_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/4315 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/4315/artifact/work/smurf.html Remote branch MDL-45678 -004-master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/4316 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/4316/artifact/work/smurf.html
            Hide
            Damyon Wiese added a comment -

            This looks like a good solution to me.

            Thanks Damien. Sending for integration.

            Show
            Damyon Wiese added a comment - This looks like a good solution to me. Thanks Damien. Sending for integration.
            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
            Damien Bezborodov added a comment -

            My branches have been rebased against Moodle upstream.

            Show
            Damien Bezborodov added a comment - My branches have been rebased against Moodle upstream.
            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 for working on this Damien,

            I'm reopening it this week for the following reasons:

            1. This could cause havoc within cron as that cache is never invalidated. It should be invalided when status is updated. This happens through enrol_plugin::update_user_enrol.
            2. I talked with Petr about this cache (Petr being the enrolments guru), he pointed out that get_enrolled_sql has rounding when dealing with the active argument (4) that rounds by -2 to improved database caching http://bit.ly/1m3Xapb. This gives a buffer of up to 100s, I think we should use that same timeframe here. You could use TTL... but I wouldn't, instead as this is just a request cache you could store a timestamp in a static variable and only use the cache if it is within 100s of that timestamp. This avoids the overhead of the ttl.
            3. You need to update the phpdocs for get_suspended_userids to reflect the new argument.
            4. You can set and retrieve arrays through MUC, there is no need to compact the array when storing it and exploding it when retrieving it, you can just give MUC an array and it will give it back to you when required.
            5. There's no need to use the 'susers-' prefix on cache items, I can't imagine us storing any other keys in this cache. Just a suggestion feel free to ignore this one.

            I think all but the last point need to be acted upon before this can land sorry.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Thanks for working on this Damien, I'm reopening it this week for the following reasons: This could cause havoc within cron as that cache is never invalidated. It should be invalided when status is updated. This happens through enrol_plugin::update_user_enrol. I talked with Petr about this cache (Petr being the enrolments guru), he pointed out that get_enrolled_sql has rounding when dealing with the active argument (4) that rounds by -2 to improved database caching http://bit.ly/1m3Xapb . This gives a buffer of up to 100s, I think we should use that same timeframe here. You could use TTL... but I wouldn't, instead as this is just a request cache you could store a timestamp in a static variable and only use the cache if it is within 100s of that timestamp. This avoids the overhead of the ttl. You need to update the phpdocs for get_suspended_userids to reflect the new argument. You can set and retrieve arrays through MUC, there is no need to compact the array when storing it and exploding it when retrieving it, you can just give MUC an array and it will give it back to you when required. There's no need to use the 'susers-' prefix on cache items, I can't imagine us storing any other keys in this cache. Just a suggestion feel free to ignore this one. I think all but the last point need to be acted upon before this can land sorry. Cheers Sam
            Hide
            CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            Damien Bezborodov added a comment -

            Hi Sam,

            Thank you for your feedback. I have implemented everything except from your second point, which confused me at first, but after some thought I came to the conclusion that it is unnecessary. The rounding of the timestamps to improve database indexing performance seems unrelated to a static cache that simply stores a list of suspended userids at a given point in time.

            The Cache API documentation recommends that instead of using TTL, that the cache should be invalidated when it is updated. I have implemented invalidation as per your feedback.

            Following the suggested logic of implementing a TTL for the 100 second time bracket, if the timetstamps were not rounded, then the TTL would be 1 second!! Clearly, using this example, our design would be flawed if it were implemented in such a fashion.

            What we are doing is caching the results for a set of users at a given point in time. It does not matter too much if we are using 1 second precision or 100 second precision. Ideally, if we were to be super-pedantic, we would not cache a list of suspended users at a given point in time, but rather cache the timestamps used for determining this, and check against time() as the clock ticks forward during program execution. But I do not think we need to be this pedantic, and we should be able to get away with simply caching a simple list of userids since this is a MODE_REQUEST cache that only persists per each page call (and page requests shouldn't run for more than a few seconds at most.)

            Let me know what you think.

            Thanks again,

            Damien

            Show
            Damien Bezborodov added a comment - Hi Sam, Thank you for your feedback. I have implemented everything except from your second point, which confused me at first, but after some thought I came to the conclusion that it is unnecessary. The rounding of the timestamps to improve database indexing performance seems unrelated to a static cache that simply stores a list of suspended userids at a given point in time. The Cache API documentation recommends that instead of using TTL, that the cache should be invalidated when it is updated. I have implemented invalidation as per your feedback. Following the suggested logic of implementing a TTL for the 100 second time bracket, if the timetstamps were not rounded, then the TTL would be 1 second!! Clearly, using this example, our design would be flawed if it were implemented in such a fashion. What we are doing is caching the results for a set of users at a given point in time . It does not matter too much if we are using 1 second precision or 100 second precision. Ideally, if we were to be super-pedantic, we would not cache a list of suspended users at a given point in time, but rather cache the timestamps used for determining this, and check against time() as the clock ticks forward during program execution. But I do not think we need to be this pedantic, and we should be able to get away with simply caching a simple list of userids since this is a MODE_REQUEST cache that only persists per each page call (and page requests shouldn't run for more than a few seconds at most.) Let me know what you think. Thanks again, Damien
            Show
            CiBoT added a comment - Results for MDL-45678 Remote repository: https://github.com/dbezborodovrp/moodle.git Remote branch MDL-45678 -006-26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/4483 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/4483/artifact/work/smurf.html Remote branch MDL-45678 -006-27 to be integrated into upstream MOODLE_27_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/4484 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/4484/artifact/work/smurf.html Remote branch MDL-45678 -006-master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/4485 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/4485/artifact/work/smurf.html
            Hide
            Damyon Wiese added a comment -

            Just noting here that in the case of cron - this "and page requests shouldn't run for more than a few seconds at most" is not true. A single cron processing enrolments especially could run for hours.

            Regardless - the default for using this new cache is false - so I don't see there being any regressions for enrolment plugins etc.

            Sending for integration - thanks Damien.

            Show
            Damyon Wiese added a comment - Just noting here that in the case of cron - this "and page requests shouldn't run for more than a few seconds at most" is not true. A single cron processing enrolments especially could run for hours. Regardless - the default for using this new cache is false - so I don't see there being any regressions for enrolment plugins etc. Sending for integration - thanks Damien.
            Hide
            Damien Bezborodov added a comment -

            Actually, that's a good point. I can check for the constant CLI_SCRIPT, and if it is checked, I can force $usecache to be false. I'll quickly update my codebase to reflect this.

            Show
            Damien Bezborodov added a comment - Actually, that's a good point. I can check for the constant CLI_SCRIPT, and if it is checked, I can force $usecache to be false. I'll quickly update my codebase to reflect this.
            Hide
            Damien Bezborodov added a comment -

            I have updated my codebase with:

                if (defined('CLI_SCRIPT') && CLI_SCRIPT) {
                    // Never use the cache for crons, as bad state could happen.
                    $usecache = false;
                }
            

            Show
            Damien Bezborodov added a comment - I have updated my codebase with: if (defined('CLI_SCRIPT') && CLI_SCRIPT) { // Never use the cache for crons, as bad state could happen. $usecache = false; }
            Show
            Damien Bezborodov added a comment - CLI_SCRIPT is defined here: https://github.com/dbezborodovrp/moodle/blob/MDL-45678-006-master/admin/cli/cron.php#L31 and here: https://github.com/dbezborodovrp/moodle/blob/MDL-45678-006-master/admin/cron.php#L47
            Hide
            Damyon Wiese added a comment -

            I don't think you should make that last change. It's up to the calling code to decide. CLI_SCRIPT !== CRON (cron can be run from admin/cron.php).

            Show
            Damyon Wiese added a comment - I don't think you should make that last change. It's up to the calling code to decide. CLI_SCRIPT !== CRON (cron can be run from admin/cron.php).
            Hide
            Damien Bezborodov added a comment -

            Hmm, okay. Would you prefer me to define a new constant CRON_SCRIPT to be more specific or just to revert my latest change entirely?

            Show
            Damien Bezborodov added a comment - Hmm, okay. Would you prefer me to define a new constant CRON_SCRIPT to be more specific or just to revert my latest change entirely?
            Hide
            Damien Bezborodov added a comment - - edited

            Also, note that admin/cron.php also defines CLI_SCRIPT even though it is ran via the Web interface.

            Show
            Damien Bezborodov added a comment - - edited Also, note that admin/cron.php also defines CLI_SCRIPT even though it is ran via the Web interface.
            Hide
            Damyon Wiese added a comment -

            I think you should revert that last change. The parameters to the function are clear - if you override them with magic then it becomes confusing.

            Show
            Damyon Wiese added a comment - I think you should revert that last change. The parameters to the function are clear - if you override them with magic then it becomes confusing.
            Hide
            Damien Bezborodov added a comment -

            I have amended my pull branches accordingly.

            Show
            Damien Bezborodov added a comment - I have amended my pull branches accordingly.
            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 for making those changes Damien, this has been integrated now.

            I did make a small commit on top of your changes to make the following adjustments:

            1. Renamed the cache from get_suspended_userids to suspended_userids.
            2. Clarify the keys and values used in the comment preceeding the cache definition in db/caches.php
            3. Fixed a typo in the phpdocs for get_suspended_userids

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Thanks for making those changes Damien, this has been integrated now. I did make a small commit on top of your changes to make the following adjustments: Renamed the cache from get_suspended_userids to suspended_userids . Clarify the keys and values used in the comment preceeding the cache definition in db/caches.php Fixed a typo in the phpdocs for get_suspended_userids Cheers Sam
            Hide
            Ankit Agarwal added a comment -

            I didn't see any noticeable difference in performance matrix, but It is quite difficult to compare these without proper isolated installs.

            The second part of the test go as expected. Passing.

            Thanks

            Show
            Ankit Agarwal added a comment - I didn't see any noticeable difference in performance matrix, but It is quite difficult to compare these without proper isolated installs. The second part of the test go as expected. Passing. Thanks
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Just in time for incoming releases, well done. And big thanks!

            We must plan for freedom,
            and not only for security,
            if for no other reason than that
            only freedom can make security secure

            –  Karl Popper

            Show
            Eloy Lafuente (stronk7) added a comment - Just in time for incoming releases, well done. And big thanks! We must plan for freedom, and not only for security, if for no other reason than that only freedom can make security secure –  Karl Popper

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: