Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.5
    • Component/s: Libraries, Performance
    • Labels:
    • Testing Instructions:
      Hide
      1. Edit your sites config.php and add "$CFG->perfdebug = 15;" if its not there already.
      2. Log in as admin
      3. Browse to settings > plugins > caching > configuration
      4. Check that you see a definition for a config cache
      5. Check you have a core/config section in the performance info area at the bottom of the page.
      6. Log out and log in as a student.
      7. Browse to a course.
      8. Check you have a core/config section in the performance info area at the bottom of the page.
      9. Browse to a module and check again.
      10. Install from scratch, without any config.php. No exceptions/errors/warnings/notices related with $CFG/config happen (display errors/logs).
      11. Upgrade to 2.5dev from any previous branch. No exceptions/errors/warnings/notices related with $CFG/config happen (display errors/logs).
      Show
      Edit your sites config.php and add "$CFG->perfdebug = 15;" if its not there already. Log in as admin Browse to settings > plugins > caching > configuration Check that you see a definition for a config cache Check you have a core/config section in the performance info area at the bottom of the page. Log out and log in as a student. Browse to a course. Check you have a core/config section in the performance info area at the bottom of the page. Browse to a module and check again. Install from scratch, without any config.php. No exceptions/errors/warnings/notices related with $CFG/config happen (display errors/logs). Upgrade to 2.5dev from any previous branch. No exceptions/errors/warnings/notices related with $CFG/config happen (display errors/logs).
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      wip-MDL-34344-m25
    • Rank:
      42713

      Description

      type of data: database records
      data structure: php object
      when it gets stored: early in setup in initialise_cfg
      where it gets stored: $CFG global
      how it gets read: php object properties
      does it need locking: don't think so..
      how it gets cleared: It doesn't really, set_config will update it if updating.
      typical sizes:
      safeguards in place (eg limits):

        Issue Links

          Activity

          Hide
          Dan Poltawski added a comment -

          Note that a big problem in this area is that plugin config isn't cached like this. So people using get_config individually to get config items suffer from lots of DB queries.

          Show
          Dan Poltawski added a comment - Note that a big problem in this area is that plugin config isn't cached like this. So people using get_config individually to get config items suffer from lots of DB queries.
          Hide
          Matt Sharpe (Inactive) added a comment -

          Obviously this could be turned off, but this would completely break dynamic config loading setups where a single Moodle installation has multiple configs based on hostname or some other variable. Is it possible to handle caching of config in this environment?

          Show
          Matt Sharpe (Inactive) added a comment - Obviously this could be turned off, but this would completely break dynamic config loading setups where a single Moodle installation has multiple configs based on hostname or some other variable. Is it possible to handle caching of config in this environment?
          Hide
          Dan Poltawski added a comment -

          Not at all, you could still do what you lied in the config.php, the configuration for the caching will need to be defined in there anyway.

          Show
          Dan Poltawski added a comment - Not at all, you could still do what you lied in the config.php, the configuration for the caching will need to be defined in there anyway.
          Hide
          Sam Hemelryk added a comment -

          Had this for development of MUC, putting it up for peer-review now.

          Show
          Sam Hemelryk added a comment - Had this for development of MUC, putting it up for peer-review now.
          Hide
          moodle.com added a comment -

          Probably risky for 2.4 ...

          Show
          moodle.com added a comment - Probably risky for 2.4 ...
          Hide
          Dan Poltawski added a comment -

          Sending all 'waiting for peer review' issues to integration before freeze, as agreed in Integrators Meeting 19/10/12. We are doing this to ensure any 'integratable issues' will not got missed before freeze..

          Show
          Dan Poltawski added a comment - Sending all 'waiting for peer review' issues to integration before freeze, as agreed in Integrators Meeting 19/10/12. We are doing this to ensure any 'integratable issues' will not got missed before freeze..
          Hide
          Dan Poltawski added a comment -

          Taking out of integration as was agreed to leave this one till start of 2.5 cycle.

          Show
          Dan Poltawski added a comment - Taking out of integration as was agreed to leave this one till start of 2.5 cycle.
          Hide
          Martin Dougiamas added a comment -

          I'd like to see this landing in 2.5 soon so it gets maximum testing.

          Show
          Martin Dougiamas added a comment - I'd like to see this landing in 2.5 soon so it gets maximum testing.
          Hide
          Martin Dougiamas added a comment -

          Damyon can you please peer review (from tomorrow, as I think Sam might update it) and submit to integration if OK?

          Show
          Martin Dougiamas added a comment - Damyon can you please peer review (from tomorrow, as I think Sam might update it) and submit to integration if OK?
          Hide
          Damyon Wiese added a comment -

          OK - I'll take a quick look today and have a better look tomorrow.

          Show
          Damyon Wiese added a comment - OK - I'll take a quick look today and have a better look tomorrow.
          Hide
          Sam Hemelryk added a comment -

          Ok I've fixed it up now, the definition and definition string were removed when we released 2.4 as this wasn't included.
          I've put them back in now and its all operational.

          As a heads up because this caches config if you want the nice hit/set/miss counters you must define $CFG->perfdebug in your config.php file.

          All yours Damyon.

          Show
          Sam Hemelryk added a comment - Ok I've fixed it up now, the definition and definition string were removed when we released 2.4 as this wasn't included. I've put them back in now and its all operational. As a heads up because this caches config if you want the nice hit/set/miss counters you must define $CFG->perfdebug in your config.php file. All yours Damyon.
          Hide
          Damyon Wiese added a comment -

          OK - I'll take a good look now.

          Show
          Damyon Wiese added a comment - OK - I'll take a good look now.
          Hide
          Damyon Wiese added a comment -

          Hi Sam,

          This looks great. I'm still looking at it but found a couple of things so far:

          Issue 1:

          get_config
          If value is not in DB - but is in $CFG->config_php_settings or $CFG->forced_plugin_settings and someone calls get_config('core/pluginname', 'thatsetting') they will get false.

          Also if value is in DB and in $CFG->config_php_settings or $CFG->forced_plugin_settings and someone calls get_config('core/pluginname', 'thatsetting') they will get the value from the DB - not the forced value.

          Issue 2 (Very minor):

          Because the cache data is such an obvious candidate for caching, there are a number of plugin specific static caches that could be changed to a thin wrapper around the new cache.

          List of candidates:
          mod/assign/locallib.php: get_admin_config()
          question/engine/bank.php: get_config()
          lib/enrollib.php: get_config()
          lib/editor/tinymce/classes/plugin.php: get_config()

          I've finished scanning the code and I'll run some tests on it now and let you know if I find anything else.

          Cheers, Damyon

          Show
          Damyon Wiese added a comment - Hi Sam, This looks great. I'm still looking at it but found a couple of things so far: Issue 1: get_config If value is not in DB - but is in $CFG->config_php_settings or $CFG->forced_plugin_settings and someone calls get_config('core/pluginname', 'thatsetting') they will get false. Also if value is in DB and in $CFG->config_php_settings or $CFG->forced_plugin_settings and someone calls get_config('core/pluginname', 'thatsetting') they will get the value from the DB - not the forced value. Issue 2 (Very minor): Because the cache data is such an obvious candidate for caching, there are a number of plugin specific static caches that could be changed to a thin wrapper around the new cache. List of candidates: mod/assign/locallib.php: get_admin_config() question/engine/bank.php: get_config() lib/enrollib.php: get_config() lib/editor/tinymce/classes/plugin.php: get_config() I've finished scanning the code and I'll run some tests on it now and let you know if I find anything else. Cheers, Damyon
          Hide
          Sam Hemelryk added a comment -

          Hi Damyon,

          1) I've just been through now looking at config_php_settings and forced_plugin_settings as I was unable to replicate the issue sorry.
          To aid me in my efforts I have created a unit test for get_config and commit that as well.
          Could you please have another look at that and just verify it is an issue, and if so a few steps so I can walk through it.

          2) I think each area like that needs to be looked at separately after this has landed.
          In looking at a couple (assign and question) I think we would need to pay careful consideration to how frequently stored config vars are used and whether there is benefit in having them available directly in memory.
          Remember that the cache provides a storage area that is accessible to everyone like the database but faster, without the bottleneck, and able to store data that doesn't fit predefined structures.
          Importantly everything in the cache is not in memory, in fact we try to minimise cache data in memory.
          Areas such as assign and question are using the cache in that they call get_config and as both store the result for use during the request (usually several times by the looks) I think the implementations are still ideal. Having to go back to the cache is going to be slower than fetching from a known variable.
          The exception would be if the return wasn't used multiple times, or if it wasn't guaranteed to be used in which case we could look at removing storage of the result and just calling get_config as needed. Either way I think a separate issue is best as an understanding of each area would be required.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Damyon, 1) I've just been through now looking at config_php_settings and forced_plugin_settings as I was unable to replicate the issue sorry. To aid me in my efforts I have created a unit test for get_config and commit that as well. Could you please have another look at that and just verify it is an issue, and if so a few steps so I can walk through it. 2) I think each area like that needs to be looked at separately after this has landed. In looking at a couple (assign and question) I think we would need to pay careful consideration to how frequently stored config vars are used and whether there is benefit in having them available directly in memory. Remember that the cache provides a storage area that is accessible to everyone like the database but faster, without the bottleneck, and able to store data that doesn't fit predefined structures. Importantly everything in the cache is not in memory, in fact we try to minimise cache data in memory. Areas such as assign and question are using the cache in that they call get_config and as both store the result for use during the request (usually several times by the looks) I think the implementations are still ideal. Having to go back to the cache is going to be slower than fetching from a known variable. The exception would be if the return wasn't used multiple times, or if it wasn't guaranteed to be used in which case we could look at removing storage of the result and just calling get_config as needed. Either way I think a separate issue is best as an understanding of each area would be required. Cheers Sam
          Hide
          Damyon Wiese added a comment -

          Hi Sam,

          No you are right - there is no problem with get_config - it looks fine to me today (and it's the same as it was yesterday). Sorry for the false alarm.

          Also - thanks for clarifying about the best use of the cache for those extra classes. I was thinking that with MUC - the idea would be to let MUC take care of all caching (including static caching) and to remove it from the other places in the code over time.

          Can you just update the branch info on the issue (You have a separate 25 branch in your repo)?

          Then it looks ready for integration to me.

          Thanks - Damyon

          Show
          Damyon Wiese added a comment - Hi Sam, No you are right - there is no problem with get_config - it looks fine to me today (and it's the same as it was yesterday). Sorry for the false alarm. Also - thanks for clarifying about the best use of the cache for those extra classes. I was thinking that with MUC - the idea would be to let MUC take care of all caching (including static caching) and to remove it from the other places in the code over time. Can you just update the branch info on the issue (You have a separate 25 branch in your repo)? Then it looks ready for integration to me. Thanks - Damyon
          Hide
          Damyon Wiese added a comment -

          Updated branches to the 25 versions and sending for integration.

          Thanks Sam!

          Show
          Damyon Wiese added a comment - Updated branches to the 25 versions and sending for integration. Thanks Sam!
          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
          Eloy Lafuente (stronk7) added a comment -

          I've reviewed all write and read raw operations along code base and it seems the patch covers all them. Also MDL-37535 has been created about one current incorrect use of the config_plugins.

          So, integrated (master only, of course), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - I've reviewed all write and read raw operations along code base and it seems the patch covers all them. Also MDL-37535 has been created about one current incorrect use of the config_plugins. So, integrated (master only, of course), thanks!
          Hide
          Frédéric Massart added a comment -

          Test passed. Just noting that I had to set $CFG->perfdebug before the inclusion of lib/setup.php.

          Show
          Frédéric Massart added a comment - Test passed. Just noting that I had to set $CFG->perfdebug before the inclusion of lib/setup.php.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Uhm, in the laptop ci server, I just noticed we are getting:

          http://stronk7.doesntexist.com/job/07.%20Run%20phpunit%20UnitTests%20(master)/1756/testReport/

          (seems related)

          Curiously the public server isn't failing there right now:

          http://integration.moodle.org/job/07.%20Run%20phpunit%20UnitTests%20(master)/317/testReport/

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Uhm, in the laptop ci server, I just noticed we are getting: http://stronk7.doesntexist.com/job/07.%20Run%20phpunit%20UnitTests%20(master)/1756/testReport/ (seems related) Curiously the public server isn't failing there right now: http://integration.moodle.org/job/07.%20Run%20phpunit%20UnitTests%20(master)/317/testReport/
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Note that in Macs the /tmp dir is an alias for /private/tmp. At some point it's not being consistently handled (link vs real) and the test fails.

          (the config.php used has dataroot defined as "/tmp/ci_dataroot-xxxx-yyyyy", aka, using the link).

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Note that in Macs the /tmp dir is an alias for /private/tmp. At some point it's not being consistently handled (link vs real) and the test fails. (the config.php used has dataroot defined as "/tmp/ci_dataroot-xxxx-yyyyy", aka, using the link).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is being solved @ MDL-37555 (applying realpath() to $CFG->phpunit_dataroot).

          Show
          Eloy Lafuente (stronk7) added a comment - This is being solved @ MDL-37555 (applying realpath() to $CFG->phpunit_dataroot).
          Hide
          Dan Poltawski added a comment -

          Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

          Show
          Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: