Evaluate navigation caching after modinfo changes

Wow, based on: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-27809-master and results @ http://moodev.com/?w=300&h=150&before=master.1307686050649&after=wip-MDL-27809-master.1307691832622#statsarray 

Really the session size reduction seems interesting... but the processing cost seems really prohibitive.

Perhaps we could limit the session cache to, say, 5 last visited courses, or something like that (or perhaps just 1). I think that could provide better balance than the current, caching off, solution. It would penalize first access to course from time to time, but improve continuous working within the last accessed courses).

Also, once again, there is one static variable there that... seems to be ok for that exact use, but we decided to stop any static caching until proper MUC arrives. Also it seems it could continue in session if we follow the "last-n" courses cached approach. it's only one small bit of information per module. Of course it will be sort of "shared memory" thing once the MUC arrives.

Ciao

Hi Eloy,

Thanks for looking at that originally, I've just been going through it now and really looking at how we can better use the caches and reduce the burden the navigation creates on the page.
Six revisions later I think I've got it - and I've run out of time.

I'm running performance tests now but would GREATLY appreciate your feedback on the changes.
Martin wants them to go in and sooner is better!

I think I worked out the serverload problem - essentially the page delivery time was dropping and because of that JMeter was sending more requests per minute.
Presently this is just a hunch, the performance testing I am doing now makes use of a throughput timer to make sure that no more than 90 requests are sent in a minute.
I am hoping that will shed some light on the problem. Either way I'll let you know how it goes as soon as I know.

Cheers
Sam

Hi Eloy,

Putting this up for integration as requested.
I ran the unit tests there was one test that needed to be corrected as we no longer store modules that extend navigation within the session cache - we now store within a static var as they are only needed when we need to regenerate the activities structure that is stored in the session cache.

I'll link some QA issues shortly.

Cheers
Sam

Unfortunately there is only one QA test for navigation, and its a usability test to do with navigating with the keyboard.

A - Tiny thing: Just saw that there are still some (6) occurrences of "count(WHATEVER->children)", surely those can be safely changed to quicker WHATEVER->count(), isn't it?

ack 'count\(.*children' lib/navigationlib.* -c

B - Aren't these 2 pieces of code redundant?

if (count($this->addedcategories) > 0) {
    list($courseselect, $courseparams) = $DB->get_in_or_equal(array_keys($this->addedcourses), SQL_PARAMS_NAMED, 'course', false);           
    $params += $courseparams;
} else {
    $courseselect = '';
}

and

list($courseids, $courseparams) = $DB->get_in_or_equal(array_keys($this->addedcourses) + array(SITEID), SQL_PARAMS_NAMED, 'lcours
e', false);

C - In a bunch of places along navigationlib we are retrieving from DB whole course and course_categories records. Not sure if we really need them complete or no, just asking:

• in load_all_categories(), lines 1405, 1425, 1433, 1441
• in initialise(), lines 2439, 2459, 2474 (seems heavy to get the whole course record for each section & activity)
• in generate_user_settings(), lines 3496, 3514 (don't we have that info somewhere else?)

D - I'm getting some debugging + failure executing lib/simpletest/testnavigationlib.php

Problem with "modinfo" data for this course
line 226 of /lib/modinfolib.php: call to debugging()
line 1071 of /lib/modinfolib.php: call to course_modinfo->__construct()
line 1566 of /lib/navigationlib.php: call to get_fast_modinfo()
line 1626 of /lib/navigationlib.php: call to global_navigation->generate_sections_and_activities()
line 362 of /lib/simpletest/testnavigationlib.php: call to global_navigation->load_generic_course_sections()
...
...

and then:

Fail: lib/simpletest/testnavigationlib.php / ▶ global_navigation_test / ▶ test_load_generic_course_sections
Equal expectation fails because [Integer: 0] differs from [Integer: 1] by 1 at [lib/simpletest/testnavigationlib.php line 363]

And that's all I've been able to find while reviewing your changes. I've been testing along a lot of pages and everything seems to work ok (comparing side by side with 2.1 without your cool patch applied).

Ciao

Some testing instructions for this would be really welcome. Some suggestions:

0) debug enabled
1) test the switchrole facility from various pages (course, some activity, participants...)
2) test the navigation on various pages (frontpage, course, some activity, admin...) and roles (admin, teacher, student...)
3) Create MDLQAs about switchrole and navigation.

Ciao

Hi Eloy,

Thanks for the feedback.

In regards to the points you've raised:

1. I've revised all uses of count ... children. There were two places where it could be improved by using the count method. Many places were within the navbar which uses an array and not a navigation_node_collection.
2. Yip redundant code indeed - I've now removed the code in block A.
3. Indeed in regards to DB queries there could certainly be future work done on reducing the number of times that the nav goes to the database and what it is requesting. I think creating a new issue to look at this would be the best way to go especially after we get some page level caching sorted out (referencing Petrs performacnce proposal aspects). As for the queries you noted:
   • 1405, 1425, 1433 Retrieving the categories full records is only done to ensure that the same fields always exist. Essentially we only ever need id, name, parent, visible, and path. (A close inspection would be required to check no more are required but I think that is it)
   • 2439, 2459, 2474 Full structures are required as they are used for require_login calls made by the navigation when loading content for AJAX.
   • 3496 By the looks of it returning id, and groupmode would be enough here and could safely be done.
   • 3514 The full user object is loaded because the code will either be $USER or the $user fetched by the query. Likely we don't need more than a handful of fields however it reduces the chance of extending code in the future hitting problems due to differences between $USER and $user.
4. I've removed a unit test case that was causing the modinfo trouble as it was attempting to fake a courses modinfo in order to lead the navigation to generate based upon the cache. This isn't really a possibility now that we have the cminfo classes. I will however create a new issue to see the navigation unit tests reviewed as I really think they need improvement.

I've created a new branch wip-MDL-27809-master-r7 for these changes and updated this issue to show that branch.
For a comparison check out https://github.com/samhemelryk/moodle/compare/wip-MDL-27809-master-r6...wip-MDL-27809-master-r7
The only other change I made was to fix a bug within the navigations ajax when navshowcategories was turned off.

Cheers
Sam

allright. thanks!

feel free to create the related issues as/when you want... I'm considering this integrated (will be in a bunch of mins).

Ciao

and this has landed integration.git, many thanks for all the hard work there!

Hi Sam,

Whilst following your testing instructions and trying to write a navigation QA test, I noticed the following small issues. (Please let me know if I should create separate MDL issues for them.)

1. When viewing the navigation block from a course page e.g. http://qa.moodle.net/course/view.php?id=2 URL and page resources have an arrow, suggesting they can be expanded (when they can't).
2. Messages and My private files appear in My courses > Features Demo > Participants > Sam Student AND in My profile. Surely they should only appear in My profile?

3. When logged in as a teacher within a course there is the option Site pages > Notes when there should only be My courses > Features Demo > Participants > Notes.

Sorry if these navigation issues are nothing at all to do with this issue - I've no idea! Please do tell me to create separate MDL issues if necessary.

Hi Helen,

A big thank you for testing this and by all means anything like the above that you spot let me know about or create issues for.
As for the things you've spotted so far I think we should open bugs for them.
All three sound like they are bugs but none of them are in areas directly affected by my changes.
The only one that may not be a bug is the notes issue, I seem to remember something about that being there so that people can create general notes however I may be wrong either way with a bug I can properly investigate it.
I will be going through the this bug creating several issues tomorrow so feel free to leave the creation of the new bugs to me or create them yourself

Cheers
Sam

Thanks Sam, you're very kind offering to create new bugs rather than being annoyed with me for not doing so!

Anyway I've created a navigation QA test - MDLQA-1169. Feedback on how it can be improved is always welcome!

Passing test without further action. Will be tested by MDLQA-1169 once this meets upstream.

Moode is now, for sure, a bit better thanks to you! This fix is now upstream.

Hi guys,

I've gone through and created 5 new bugs for the different things discussed here:

1. MDL-28064 Look for way to reduce the number of times the navigation goes to the database and reduce the amount of information it is retrieving
2. MDL-28065 Review and improve navigaiton unit tests
3. MDL-28066 Resources in the navigation that don't add any extra are still showing as expandable branches
4. MDL-28067 Messages and My private files appear in My courses > Features Demo > Participants > Sam Student AND in My profile
5. MDL-28068 Review the notes links within the navigation

Cheers
Sam

I thought I should mention, I've had a client report a couple of bugs caused by the code change for this ticket. One (which I've already logged as MDL-28934) is that the "My Courses" list in the Navigation block has problems if you turn on $CFG->navshowcategories and $CFG->navshowallcourses.

The other problem (which I haven't pinned down enough to file a ticket for yet) is that, again with $CFG->navshowcategories and $CFG->navshowallcourses turned on, not all the categories are showing in the "Courses" list of the Navigation block. I haven't had a chance to dig into the problem yet, but I do find that if I set $CFG->navcourselimit to a really high number, then all the categories show. So, I'd suspect there's some code in there that's not respecting the setting for $CFG->navshowallcourses.

Both of these problems go away if I revert commit 1f7907dac31cfd52a3.

Cheers,
Aaron

Okay, I've been able to replicate the issue with course categories sometimes not showing in the Navigation block. I've filed it as MDL-28967.

I've also filed a separate bug, MDL-28965, about the $CFG->navshowallcourses setting not actually showing all the courses. But that behavior persists even if I roll back to before the MDL-27809 changes, so it seems to be unrelated. (I think it may actually just be a matter of the help text for $CFG->navshowallcourses incorrectly describing its intended behavior.)

Cheers,
Aaron

Hi Aaron, thanks for reporting those bugs and digging into them to work out how to replicate them.
Definitely filling the new issues is the right way to go about things, I'll look into the new issues when I get a chance.

Cheers
Sam