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

          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