Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-32377

prevent string cache rebuild when non-existent string requested

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1, 2.2
    • Fix Version/s: 2.3
    • Component/s: Libraries
    • Testing Instructions:
      Hide

      For developers:
      1/ remove some string from core lang pack string cache
      2/ verify moodle does not complain about missing string
      3/ rebuild cache manually and verify the string reappeared in the cache

      Show
      For developers: 1/ remove some string from core lang pack string cache 2/ verify moodle does not complain about missing string 3/ rebuild cache manually and verify the string reappeared in the cache
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w16_MDL-32377_m23_strcache

      Description

      reported in MDL-31904

        Gliffy Diagrams

          Issue Links

            Activity

            skodak Petr Skoda created issue -
            skodak Petr Skoda made changes -
            Field Original Value New Value
            Link This issue has a non-specific relationship to MDL-31904 [ MDL-31904 ]
            skodak Petr Skoda made changes -
            Fix Version/s STABLE backlog [ 10463 ]
            Labels performance triaged
            skodak Petr Skoda made changes -
            Status Open [ 1 ] Development in progress [ 3 ]
            skodak Petr Skoda made changes -
            Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
            Pull Master Diff URL https://github.com/skodak/moodle/compare/master...w16_MDL-32377_m23_strcache
            Pull Master Branch w16_MDL-32377_m23_strcache
            Pull from Repository git://github.com/skodak/moodle.git
            Fix Version/s 2.2.3 [ 12053 ]
            Fix Version/s 2.3 [ 10657 ]
            Fix Version/s STABLE backlog [ 10463 ]
            Testing Instructions For developers:
            1/ remove some string from core lang pack string cache
            2/ verify moodle does not complain about missing string
            3/ rebuild cache manually and verify the string reappeared in the cache
            skodak Petr Skoda made changes -
            Fix Version/s 2.2.3 [ 12053 ]
            samhemelryk Sam Hemelryk made changes -
            Currently in integration Yes [ 10041 ]
            poltawski Dan Poltawski made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Integrator poltawski
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Petr,

            I'm pondering this issue in my mind

            1. I think that load_component_strings() $disablecache and $cacheresult are really quite similar sounding arguments. I understand the distinction but wonder if there is a way to make this clearer as the difference is quite subtle.
            2. Should we retain this behaviour in DEBUG_DEVELOPER? What are the cases when we do rebuild cache?
            Show
            poltawski Dan Poltawski added a comment - Hi Petr, I'm pondering this issue in my mind I think that load_component_strings() $disablecache and $cacheresult are really quite similar sounding arguments. I understand the distinction but wonder if there is a way to make this clearer as the difference is quite subtle. Should we retain this behaviour in DEBUG_DEVELOPER? What are the cases when we do rebuild cache?
            Hide
            skodak Petr Skoda added a comment - - edited

            The difference is subtle - see linked MDL that explains the chained cache rebuild trouble, I hope this could actually eliminate it without file locking. The problem here is that there is no separate cache reset and there are hacks that rely on the current $disable cache mechanism - I had to either undo those hacks (such as the help.php) and change API or add new fully BC parameter.

            Hmm, now I think I should have probable created new method for cache reset and redefine the meaning of $disable cache... reopening, I need one more week to think about this, thanks a lot!!!

            Show
            skodak Petr Skoda added a comment - - edited The difference is subtle - see linked MDL that explains the chained cache rebuild trouble, I hope this could actually eliminate it without file locking. The problem here is that there is no separate cache reset and there are hacks that rely on the current $disable cache mechanism - I had to either undo those hacks (such as the help.php) and change API or add new fully BC parameter. Hmm, now I think I should have probable created new method for cache reset and redefine the meaning of $disable cache... reopening, I need one more week to think about this, thanks a lot!!!
            skodak Petr Skoda made changes -
            Status Integration review in progress [ 10004 ] Reopened [ 4 ]
            Hide
            skodak Petr Skoda added a comment -

            Resubmitting, I have used different trick that did not require changes in API.

            I have also fixed another related problem that $disablelocal==true was borking cache contents and might incorrectly use normal cached results...

            Show
            skodak Petr Skoda added a comment - Resubmitting, I have used different trick that did not require changes in API. I have also fixed another related problem that $disablelocal==true was borking cache contents and might incorrectly use normal cached results...
            skodak Petr Skoda made changes -
            Status Reopened [ 4 ] Waiting for integration review [ 10010 ]
            Hide
            skodak Petr Skoda added a comment -

            Note: this patch expects that admins do not add strings to 'en' lang packs on production sites and that plugin developers bump up version numbers after introduction of new lang strings. In the worst case scenario perf might be lower on pages that use these new strings until admin resets caches. On the other hand this could eliminate problems described in MDL-31904 and it improves performance on pages that are using missing strings because the cache in not rewritten on each get_string('missingstring') call.

            Show
            skodak Petr Skoda added a comment - Note: this patch expects that admins do not add strings to 'en' lang packs on production sites and that plugin developers bump up version numbers after introduction of new lang strings. In the worst case scenario perf might be lower on pages that use these new strings until admin resets caches. On the other hand this could eliminate problems described in MDL-31904 and it improves performance on pages that are using missing strings because the cache in not rewritten on each get_string('missingstring') call.
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Petr,

            I was discussing this with Eloy and well, i'll just paste in what he commented upon:

            1) Requiring upgrade on lang changes needs to be documented, because that was not necessary until now.
            2) string_exists(), used here and there, always use cache, and never recalculates component strings. So it is possible for string_exists() to be returning false, but the corresponding get_string() be returning one proper string. No critical, but for your consideration.
            3) Haven't looked to how the reset() happens. If it's simply deleting files, apparently np. But if it's done by calling load_component_strings() perhaps it would need some change. In any case, I think we should have better mechanism. I can see race conditions rebuilding those caches for sure.

            Looking at 2) help.php seems to be particularly trying to do inteligence about reloading strings relating to that

            Show
            poltawski Dan Poltawski added a comment - Hi Petr, I was discussing this with Eloy and well, i'll just paste in what he commented upon: 1) Requiring upgrade on lang changes needs to be documented, because that was not necessary until now. 2) string_exists(), used here and there, always use cache, and never recalculates component strings. So it is possible for string_exists() to be returning false, but the corresponding get_string() be returning one proper string. No critical, but for your consideration. 3) Haven't looked to how the reset() happens. If it's simply deleting files, apparently np. But if it's done by calling load_component_strings() perhaps it would need some change. In any case, I think we should have better mechanism. I can see race conditions rebuilding those caches for sure. Looking at 2) help.php seems to be particularly trying to do inteligence about reloading strings relating to that
            Hide
            skodak Petr Skoda added a comment -

            1) it is necessary to bump up version.php after string change since 2.0 - I was telling that to everybody during reviews
            2) yes, string_exists() has to always use cache for performance reasons, it is used too much in stable code; this is the main reason for 1)
            3) unfortunately that is again related to 1) - see help.php nasty tricks

            I do not know where to document the requirement for version bumps on en string change. Please note it is not just adding of new en strings, the cache is outdated when you modify any en string too, but thanks to the constant resetting ppl might not notice it.

            You have to bump up string if:

            • change db structure
            • change cron
            • change permissions
            • change en strings
            • change javascript

            I am going to update http://docs.moodle.org/dev/Upgrade_API now

            Show
            skodak Petr Skoda added a comment - 1) it is necessary to bump up version.php after string change since 2.0 - I was telling that to everybody during reviews 2) yes, string_exists() has to always use cache for performance reasons, it is used too much in stable code; this is the main reason for 1) 3) unfortunately that is again related to 1) - see help.php nasty tricks I do not know where to document the requirement for version bumps on en string change. Please note it is not just adding of new en strings, the cache is outdated when you modify any en string too, but thanks to the constant resetting ppl might not notice it. You have to bump up string if: change db structure change cron change permissions change en strings change javascript I am going to update http://docs.moodle.org/dev/Upgrade_API now
            Hide
            skodak Petr Skoda added a comment -

            done, upgrade docs updated

            Show
            skodak Petr Skoda added a comment - done, upgrade docs updated
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            (thanks, looks perfect)

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - (thanks, looks perfect)
            poltawski Dan Poltawski made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks, this has been integrated - going to test when I get home

            Show
            poltawski Dan Poltawski added a comment - Thanks, this has been integrated - going to test when I get home
            poltawski Dan Poltawski made changes -
            Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
            poltawski Dan Poltawski made changes -
            Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
            Tester poltawski
            Hide
            poltawski Dan Poltawski added a comment -

            Passing works as described.

            Note you get the warnings about missing strings on purge-caches screen, assume that is expected.

            Show
            poltawski Dan Poltawski added a comment - Passing works as described. Note you get the warnings about missing strings on purge-caches screen, assume that is expected.
            poltawski Dan Poltawski made changes -
            Status Testing in progress [ 10011 ] Tested [ 10006 ]
            Hide
            andyjdavis Andrew Davis added a comment -

            I just tested this and didnt get any missing strings message on the purge cache screen. Although the strings I removed from the cache were not used on the purge cache screen.

            Show
            andyjdavis Andrew Davis added a comment - I just tested this and didnt get any missing strings message on the purge cache screen. Although the strings I removed from the cache were not used on the purge cache screen.
            Hide
            poltawski Dan Poltawski added a comment -

            Mine was on the purge cache screen (block title)

            Show
            poltawski Dan Poltawski added a comment - Mine was on the purge cache screen (block title)
            Hide
            poltawski Dan Poltawski added a comment -

            Bonza mate!

            Your changes have made it into the Moodle release! Its time to celebrate! Put a shrimp on the barbie and grab a stubby.

            Hooroo

            Show
            poltawski Dan Poltawski added a comment - Bonza mate! Your changes have made it into the Moodle release! Its time to celebrate! Put a shrimp on the barbie and grab a stubby. Hooroo
            poltawski Dan Poltawski made changes -
            Status Tested [ 10006 ] Closed [ 6 ]
            Resolution Fixed [ 1 ]
            Currently in integration Yes [ 10041 ]
            poltawski Dan Poltawski made changes -
            Integration date 19/Apr/12
            Hide
            aolley Adam Olley added a comment - - edited

            It's extremely unfortunate that this means that a simple string change will require an outage of the Moodle instance, which, ideally, should only be happening for database changes or upgrade code.

            It'd be nice if upgrade only caused an outage if it was going to touch those types of things. But that's really a seperate issue than here.

            Show
            aolley Adam Olley added a comment - - edited It's extremely unfortunate that this means that a simple string change will require an outage of the Moodle instance, which, ideally, should only be happening for database changes or upgrade code. It'd be nice if upgrade only caused an outage if it was going to touch those types of things. But that's really a seperate issue than here.
            Subversion JIRA

            Links Hierarchy

             Documentation

            Invalid license: EXPIRED

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  25/Jun/12