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

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

        Gliffy Diagrams

          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: