Moodle
  1. Moodle
  2. MDL-25981

Refactor code around get_fast_modinfo to improve quality, add plugin hooks

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.1
    • Fix Version/s: 2.0.2
    • Component/s: Course
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      15380

      Description

      After earlier work (see MDL-24631) and discussion with interested people (including key suggestions from Petr), we came up with a new proposal to improve get_fast_modinfo:

      http://docs.moodle.org/en/Development:Module_visibility_and_display

      This bug is about implementing the parts of that proposal which only clean up existing behaviour and provide a few extra hooks for plugin developers, without adding any new features or changing the way standard Moodle works; in other words, with no change to the logic about visibility.

      1. MDL-25981_test_plan.txt
        5 kB
        Sam Marshall
      2. url.patch
        2 kB
        Sam Marshall

        Issue Links

          Activity

          Hide
          Sam Marshall added a comment -

          This test plan describes how to test the changes. It also explains which of the changes in the commit are related to which aspects of the change. [These 'aspects' aren't independent, so they aren't done as separate commits.]

          I put the test plan here not in the PULL request in case it needs to be reused by multiple PULL requests.

          Show
          Sam Marshall added a comment - This test plan describes how to test the changes. It also explains which of the changes in the commit are related to which aspects of the change. [These 'aspects' aren't independent, so they aren't done as separate commits.] I put the test plan here not in the PULL request in case it needs to be reused by multiple PULL requests.
          Hide
          Sam Marshall added a comment -

          I profiled 20 requests for course page, by user with debug off, 'before' and 'after' this change. As our dev server is shared this is kind of sketchy (a lot of variation) but the median changed from 374 to 377ms.

          While this is a very slight slowdown, I also submitted a performance enhancement MDL-26044 which was designed in conjunction with this request and mitigates (or possibly removes) the difference by enhancing performance of moodle_url (one of the aspects of this change is a slight increase in use of moodle_url).

          Show
          Sam Marshall added a comment - I profiled 20 requests for course page, by user with debug off, 'before' and 'after' this change. As our dev server is shared this is kind of sketchy (a lot of variation) but the median changed from 374 to 377ms. While this is a very slight slowdown, I also submitted a performance enhancement MDL-26044 which was designed in conjunction with this request and mitigates (or possibly removes) the difference by enhancing performance of moodle_url (one of the aspects of this change is a slight increase in use of moodle_url).
          Hide
          Sam Marshall added a comment -

          I also profiled a forum view page - not so much changed on this page but the require_login changes do apply (and of course get_fast_modinfo is also used for nav block). This time I did 10 requests of each type. Unfortunately the results were not significant (well they showed the new code as being 4ms faster by the same median measurement, but I don't believe this). At any rate it doesn't look like there is an unacceptable slowdown there either.

          Show
          Sam Marshall added a comment - I also profiled a forum view page - not so much changed on this page but the require_login changes do apply (and of course get_fast_modinfo is also used for nav block). This time I did 10 requests of each type. Unfortunately the results were not significant (well they showed the new code as being 4ms faster by the same median measurement, but I don't believe this). At any rate it doesn't look like there is an unacceptable slowdown there either.
          Hide
          Sam Marshall added a comment -

          sorry for so many comments, just finishing this off with some final notes (er final for now, until people review it)...

          1. If this change is accepted into core (which by the way, from my point of view, it would be very nice if it goes into 2.0 branch, but if somebody decides it should go into 2.1 that's okay), I propose to update the existing MoodleDocs page linked above to turn it into developer documentation for this area of code.

          2. While making the change I noticed two opportunities for performance enhancements which could be applied later:

          i. If the existing $USER variable that caches group membership in session were updated to add one that has grouping information, this would remove the need for a db query in get_fast_modinfo where groupmembersonly mode is used.

          ii. Using get_record to get the $cm in most requests (within module code etc) is inefficient because we also call get_fast_modinfo on pretty much all pages, and this could also obtain the $cm without making an additional db request.

          Show
          Sam Marshall added a comment - sorry for so many comments, just finishing this off with some final notes (er final for now, until people review it)... 1. If this change is accepted into core (which by the way, from my point of view, it would be very nice if it goes into 2.0 branch, but if somebody decides it should go into 2.1 that's okay), I propose to update the existing MoodleDocs page linked above to turn it into developer documentation for this area of code. 2. While making the change I noticed two opportunities for performance enhancements which could be applied later: i. If the existing $USER variable that caches group membership in session were updated to add one that has grouping information, this would remove the need for a db query in get_fast_modinfo where groupmembersonly mode is used. ii. Using get_record to get the $cm in most requests (within module code etc) is inefficient because we also call get_fast_modinfo on pretty much all pages, and this could also obtain the $cm without making an additional db request.
          Hide
          Sam Marshall added a comment - - edited

          After comments on the previous attempt (which I will update to fix all those issues later this week), the issue of ->extra and its continued usage in resource modules came up. Here are some possible solutions.

          OPTION 1
          --------

          $info->jsurl (url to open if link is activated by javascript; not set if no special JS behaviour) *
          $info->jswindow (WINDOW_SAME**, WINDOW_NEW, WINDOW_POPUP)
          $info->jswidth, ->jsheight (used only if WINDOW_POPUP; if not specified default to 620, 450)

          ref ** Query: What is RESOURCELIB_DISPLAY_OPEN for? It seems to just open the url with ?redirect=1, do we need to use javascript for that or is this same as clicking it without? Not sure what ?redirect does.

          ref * Query: Hopefully it is ok to go to the 'normal' url if javascript is not on? Because that's what it does.

          OPTION 2
          --------

          This is simpler, basically we can just replace the 'extra' which I don't like because it seems a bit hacky, with a specific function:

          $info->onclick (text of onclick)

          OPTION 3
          --------

          Leave the system as it is but un-deprecate extra (& add it to the 'class' version).

          MODINFO SIZE
          ------------

          Not related to the above and could definitely be left for later if that's sensible. But just something else I noticed.

          If adding fields, may also be good time to check that modinfo is 'minimised' in db e.g when saving to database, we only include in the serialised object any fields which have non-default value (like if grouping is 0 we don't save it, if extra is '' we don't save it, that kind of thing) and reconstruct using empty() ? : type syntax when constructing in memory, as done for some fields already.

          Our typical (median) modinfo is about 22KB. That's quite long for one field. I think it can probably be reduced by a significant proportion (maybe even 50%+) if we just don't store unused fields. Not sure that transferring 20KB from database each time we get a course record really causes a problem, though, so this might not particularly be a big performance win, just seems sensible.

          22KB is typical (median), but just for amusement, here is the OU live system top 10 modinfo length (characters):

          1156226 (woo, we broke the 1MB barrier!)
          722141
          408022
          345936
          309951
          305060
          297707
          289248
          284914
          269925

          these are in our 1.9 (+ OU bits, some of which do add to modinfo) but I don't think the code is much different in 2... the #1 course there has 1449 course modules (incidentally most of these are labels), while #10 has only 213... I don't have convenient test code to create a course with 1000 activities in moodle 2 but I suspect the result would be similar.

          Show
          Sam Marshall added a comment - - edited After comments on the previous attempt (which I will update to fix all those issues later this week), the issue of ->extra and its continued usage in resource modules came up. Here are some possible solutions. OPTION 1 -------- $info->jsurl (url to open if link is activated by javascript; not set if no special JS behaviour) * $info->jswindow (WINDOW_SAME**, WINDOW_NEW, WINDOW_POPUP) $info->jswidth, ->jsheight (used only if WINDOW_POPUP; if not specified default to 620, 450) ref ** Query: What is RESOURCELIB_DISPLAY_OPEN for? It seems to just open the url with ?redirect=1, do we need to use javascript for that or is this same as clicking it without? Not sure what ?redirect does. ref * Query: Hopefully it is ok to go to the 'normal' url if javascript is not on? Because that's what it does. OPTION 2 -------- This is simpler, basically we can just replace the 'extra' which I don't like because it seems a bit hacky, with a specific function: $info->onclick (text of onclick) OPTION 3 -------- Leave the system as it is but un-deprecate extra (& add it to the 'class' version). MODINFO SIZE ------------ Not related to the above and could definitely be left for later if that's sensible. But just something else I noticed. If adding fields, may also be good time to check that modinfo is 'minimised' in db e.g when saving to database, we only include in the serialised object any fields which have non-default value (like if grouping is 0 we don't save it, if extra is '' we don't save it, that kind of thing) and reconstruct using empty() ? : type syntax when constructing in memory, as done for some fields already. Our typical (median) modinfo is about 22KB. That's quite long for one field. I think it can probably be reduced by a significant proportion (maybe even 50%+) if we just don't store unused fields. Not sure that transferring 20KB from database each time we get a course record really causes a problem, though, so this might not particularly be a big performance win, just seems sensible. 22KB is typical (median), but just for amusement, here is the OU live system top 10 modinfo length (characters): 1156226 (woo, we broke the 1MB barrier!) 722141 408022 345936 309951 305060 297707 289248 284914 269925 these are in our 1.9 (+ OU bits, some of which do add to modinfo) but I don't think the code is much different in 2... the #1 course there has 1449 course modules (incidentally most of these are labels), while #10 has only 213... I don't have convenient test code to create a course with 1000 activities in moodle 2 but I suspect the result would be similar.
          Hide
          Petr Škoda added a comment -

          OPTION 1

          The RESOURCELIB_DISPLAY_OPEN means that the resource thing should just open, the redirection through the view.php is used because we need to do the logging and course completion - all the course links go through the view.php.


          I personally like the OPTION 2 for now because it is simple and describes exactly what is going on.


          My +10 to compress the modinfo by eliminating the defaults. In 2.1 we may even change the format completely.

          thanks

          Show
          Petr Škoda added a comment - OPTION 1 The RESOURCELIB_DISPLAY_OPEN means that the resource thing should just open, the redirection through the view.php is used because we need to do the logging and course completion - all the course links go through the view.php. I personally like the OPTION 2 for now because it is simple and describes exactly what is going on. My +10 to compress the modinfo by eliminating the defaults. In 2.1 we may even change the format completely. thanks
          Hide
          Sam Marshall added a comment -

          Great, thanks, I'll implement option 2. I like the simple part as well

          Show
          Sam Marshall added a comment - Great, thanks, I'll implement option 2. I like the simple part as well
          Hide
          Sam Marshall added a comment -

          Ooops. To have it on record, I rewrote history so this may disappear at some point (sorry I guess I should have made a new branch) - until then Petr's comments and my responses to them are available here:

          https://github.com/sammarshallou/moodle/commit/aca388b1033ad8bb986073c8fd6ee46181fc0926#comments

          basically action has been taken on all comments except where agreed, and except that I still need to fix the label in course/resources.php which change isn't in the main git yet ( https://github.com/skodak/moodle/compare/master...w04_MDL-26041_20_resources )

          My changes re petr's comments are here:

          https://github.com/sammarshallou/moodle/commit/506488954c0610e0f5aac7202cc4cb314e17ce51

          and as a separate commit I also reduced the modinfo size when building new modinfo. In my testing, this reduced modinfo size from 5,453 to 2,843 bytes on a small test course, so a 48% size reduction. Not bad. (The actual % depends on whether you have conditional availability enabled, you probably save less if you don't.)

          https://github.com/sammarshallou/moodle/commit/71247b1920de0cc042cee34a6880f1566c142967

          Once this week's updates are published I will rebase, in a new branch, fix the remaining problem(s) and merge the 'fixing petr's comments' commits into the main one so it makes more sense in core history. And then file another PULL request!

          Show
          Sam Marshall added a comment - Ooops. To have it on record, I rewrote history so this may disappear at some point (sorry I guess I should have made a new branch) - until then Petr's comments and my responses to them are available here: https://github.com/sammarshallou/moodle/commit/aca388b1033ad8bb986073c8fd6ee46181fc0926#comments basically action has been taken on all comments except where agreed, and except that I still need to fix the label in course/resources.php which change isn't in the main git yet ( https://github.com/skodak/moodle/compare/master...w04_MDL-26041_20_resources ) My changes re petr's comments are here: https://github.com/sammarshallou/moodle/commit/506488954c0610e0f5aac7202cc4cb314e17ce51 and as a separate commit I also reduced the modinfo size when building new modinfo. In my testing, this reduced modinfo size from 5,453 to 2,843 bytes on a small test course, so a 48% size reduction. Not bad. (The actual % depends on whether you have conditional availability enabled, you probably save less if you don't.) https://github.com/sammarshallou/moodle/commit/71247b1920de0cc042cee34a6880f1566c142967 Once this week's updates are published I will rebase, in a new branch, fix the remaining problem(s) and merge the 'fixing petr's comments' commits into the main one so it makes more sense in core history. And then file another PULL request!
          Hide
          Sam Marshall added a comment - - edited

          This (grr - 'this' = attached url.patch - why does it now show that information with the attach comment?) is the change I made to the 'url' module in order to test that the new ->onclick feature works.

          To demonstrate backward compatibility, I haven't actually changed any of the modules to use the new cached_cm_info class when returning their data (so this patch has not been applied in my github branches). I can do that if desired; it may be best as a separate change.

          Show
          Sam Marshall added a comment - - edited This (grr - 'this' = attached url.patch - why does it now show that information with the attach comment?) is the change I made to the 'url' module in order to test that the new ->onclick feature works. To demonstrate backward compatibility, I haven't actually changed any of the modules to use the new cached_cm_info class when returning their data (so this patch has not been applied in my github branches). I can do that if desired; it may be best as a separate change.
          Hide
          Dan Poltawski added a comment -

          MDL-722 is a bit of history for you

          Show
          Dan Poltawski added a comment - MDL-722 is a bit of history for you
          Hide
          Tim Hunt added a comment -

          Surely this bug is fixed? Why is it still open?

          Show
          Tim Hunt added a comment - Surely this bug is fixed? Why is it still open?
          Hide
          Sam Marshall added a comment -

          Yes it was fixed on Jan 26th (main commit) which I think was 2.0.2.

          Show
          Sam Marshall added a comment - Yes it was fixed on Jan 26th (main commit) which I think was 2.0.2.

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: