Moodle
  1. Moodle
  2. MDL-32377

prevent string cache rebuild when non-existent string requested

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      39233

      Description

      reported in MDL-31904

        Issue Links

          Activity

          Hide
          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
          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
          Petr Škoda 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
          Petr Škoda 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!!!
          Hide
          Petr Škoda 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
          Petr Škoda 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...
          Hide
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda added a comment -

          done, upgrade docs updated

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

          (thanks, looks perfect)

          Show
          Eloy Lafuente (stronk7) added a comment - (thanks, looks perfect)
          Hide
          Dan Poltawski added a comment -

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

          Show
          Dan Poltawski added a comment - Thanks, this has been integrated - going to test when I get home
          Hide
          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
          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.
          Hide
          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
          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
          Dan Poltawski added a comment -

          Mine was on the purge cache screen (block title)

          Show
          Dan Poltawski added a comment - Mine was on the purge cache screen (block title)
          Hide
          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
          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
          Hide
          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
          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.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: