Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 2.3, 2.4.1
    • Fix Version/s: 2.4.2
    • Component/s: Caching, Filters
    • Labels:
    • Testing Instructions:
      Hide
      1. Make sure you have cachetext off.
      2. Make sure you have perfdebug on.
      3. Create a course, add a forum, and enrol a student.
      4. Log in as the student.
      5. Enter the forum and make a post that contains HTML (make something bold and something else italic)
      6. Submit the post and then go back to view it.
      7. Check that down you have an entry for core/htmlpurifier in the perfdebug info.
      Show
      Make sure you have cachetext off. Make sure you have perfdebug on. Create a course, add a forum, and enrol a student. Log in as the student. Enter the forum and make a post that contains HTML (make something bold and something else italic) Submit the post and then go back to view it. Check that down you have an entry for core/htmlpurifier in the perfdebug info.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      wip-MDL-36297-m25

      Description

      It should be relatively simple to add a cache to purify_html() in weblib.php and it might give us a dramatic win.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              dougiamas Martin Dougiamas added a comment -

              Sam, could you please make a small patch for this and see if it makes a difference?

              My feeling is that it will be huge on sites with significant numbers of texts and if so I'd like to see it in 2.4

              Show
              dougiamas Martin Dougiamas added a comment - Sam, could you please make a small patch for this and see if it makes a difference? My feeling is that it will be huge on sites with significant numbers of texts and if so I'd like to see it in 2.4
              Hide
              dougiamas Martin Dougiamas added a comment -

              Note this cache will only need to be invalidated on Moodle upgrades (that is the only time the HTML purifier code could change and make a cache possibly invalid).

              Show
              dougiamas Martin Dougiamas added a comment - Note this cache will only need to be invalidated on Moodle upgrades (that is the only time the HTML purifier code could change and make a cache possibly invalid).
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Hi Petr,

              I've added you as a watcher here as you may be able to help me with this. I've spent minimal time on this and before I really start digging I thought I would ask, I have a lot going on at the moment between this, caching, navigation, and integration.

              I've implemented a simple cache to sit around HTML purifier however at the moment it appears to have minimal effect, it appears the text caching is being hit predominantly.
              In thinking about it perhaps this is expected, text caching is user+context specific correct? so the rational is that in caching the HTML purified text we are offsetting that call of that. In which case this could be tricky to test/quantify.
              Just wondering whether you have any ideas here, is it worth looking to move the text cache to MUC at the same time, and if we do is there any real point in having the HTML purifier text cached as well.

              Any thoughts appreciated?

              Many thanks
              Sam

              Show
              samhemelryk Sam Hemelryk added a comment - Hi Petr, I've added you as a watcher here as you may be able to help me with this. I've spent minimal time on this and before I really start digging I thought I would ask, I have a lot going on at the moment between this, caching, navigation, and integration. I've implemented a simple cache to sit around HTML purifier however at the moment it appears to have minimal effect, it appears the text caching is being hit predominantly. In thinking about it perhaps this is expected, text caching is user+context specific correct? so the rational is that in caching the HTML purified text we are offsetting that call of that. In which case this could be tricky to test/quantify. Just wondering whether you have any ideas here, is it worth looking to move the text cache to MUC at the same time, and if we do is there any real point in having the HTML purifier text cached as well. Any thoughts appreciated? Many thanks Sam
              Show
              samhemelryk Sam Hemelryk added a comment - btw current work: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-36297-m24
              Hide
              dougiamas Martin Dougiamas added a comment -

              Firstly, the really strong benefit will be seen when filters are all converted to use MUC as well and we can get rid of text cache, but that is too much for now. Text cache has too many variables and gets invalidated too easily.

              In 2.4 though, this patch will help sites that don't use text cache only for short times or not at all (and I've heard some really big ones don't because the db traffic is a lot), as well as every time text cache needs to be rebuilt. Main thing here is that we should at least not be slower than before.

              It might be interesting to convert text_cache to MUC as well for 2.4 ... might save a lot of db access for some sites.

              Admins will have some real tools to tune with.

              Show
              dougiamas Martin Dougiamas added a comment - Firstly, the really strong benefit will be seen when filters are all converted to use MUC as well and we can get rid of text cache, but that is too much for now. Text cache has too many variables and gets invalidated too easily. In 2.4 though, this patch will help sites that don't use text cache only for short times or not at all (and I've heard some really big ones don't because the db traffic is a lot), as well as every time text cache needs to be rebuilt. Main thing here is that we should at least not be slower than before. It might be interesting to convert text_cache to MUC as well for 2.4 ... might save a lot of db access for some sites. Admins will have some real tools to tune with.
              Hide
              poltawski Dan Poltawski added a comment -

              Related to this topic, i've just spotted MDL-36034 in the peer review queue.

              Show
              poltawski Dan Poltawski added a comment - Related to this topic, i've just spotted MDL-36034 in the peer review queue.
              Hide
              poltawski Dan Poltawski added a comment -

              Been playing with this, but I can't see any change being logged in the footer, no hits/misses or anything about it being used.

              Show
              poltawski Dan Poltawski added a comment - Been playing with this, but I can't see any change being logged in the footer, no hits/misses or anything about it being used.
              Hide
              dougiamas Martin Dougiamas added a comment -

              Do you have text caching off?

              Show
              dougiamas Martin Dougiamas added a comment - Do you have text caching off?
              Hide
              poltawski Dan Poltawski added a comment -

              Doh! In my config.php: $CFG->cachetext = 0;

              Show
              poltawski Dan Poltawski added a comment - Doh! In my config.php: $CFG->cachetext = 0;
              Hide
              poltawski Dan Poltawski added a comment -

              Just thought about that and realised that cachetext = 0 should be expected to work!

              Show
              poltawski Dan Poltawski added a comment - Just thought about that and realised that cachetext = 0 should be expected to work!
              Hide
              poltawski Dan Poltawski added a comment -

              I have text caching disabled and it seems a lot to me like we are hardly ever running through html purifier at all..

              Seemingly this is because is_purify_html_necessary() often deems it unnecessary, if I remove that check we're running purify_html on most pages at least, but without it i'm seeing it seldom hit.

              Show
              poltawski Dan Poltawski added a comment - I have text caching disabled and it seems a lot to me like we are hardly ever running through html purifier at all.. Seemingly this is because is_purify_html_necessary() often deems it unnecessary, if I remove that check we're running purify_html on most pages at least, but without it i'm seeing it seldom hit.
              Hide
              poltawski Dan Poltawski added a comment -

              I deployed the patch to the moodle.org clone and disabled the text caching there to see if it was more utilised with 'real data'

              Show
              poltawski Dan Poltawski added a comment - I deployed the patch to the moodle.org clone and disabled the text caching there to see if it was more utilised with 'real data'
              Hide
              stronk7 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
              stronk7 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
              poltawski Dan Poltawski added a comment -

              I've integrated this now, thanks Sam.

              As I said, I don't see it making a dramatic difference, but also it seems to wor nicely.

              Show
              poltawski Dan Poltawski added a comment - I've integrated this now, thanks Sam. As I said, I don't see it making a dramatic difference, but also it seems to wor nicely.
              Hide
              poltawski Dan Poltawski added a comment -

              Doh. Please could you add testing instructions Sam?

              Show
              poltawski Dan Poltawski added a comment - Doh. Please could you add testing instructions Sam?
              Hide
              rwijaya Rossiani Wijaya added a comment -

              This works as expected.

              Tested in 2.4 and master

              Test passed.

              Show
              rwijaya Rossiani Wijaya added a comment - This works as expected. Tested in 2.4 and master Test passed.
              Hide
              poltawski 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
              poltawski 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:
                  8 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    11/Mar/13