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

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!

> 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.

> 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.

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

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.

This seems to have been introduced in MDL-40218.

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.

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

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.

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

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.

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

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

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.

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.

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.

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

This looks like a good solution to me.

Thanks Damien. Sending for integration.

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

My branches have been rebased against Moodle upstream.

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

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

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

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

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

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.

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.

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;

    }

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

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).

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?

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

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.

I have amended my pull branches accordingly.

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

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

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

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