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

optimise static file caching - fix headers and use slash arguments

    Details

    • Testing Instructions:
      Hide

      We need to test that we are not breaking file servering anywhere and that we are not ending up with broken links/resources, so there there are several parameters that affect static file serving and caching which will be need to be tried disabeld and enabled:

      1/ $CFG->slasharguments
      2/ $CFG->cachejs
      3/ $CFG->themedesignermode
      4/ $CFG->yuicomboloading
      5/ ssl mode
      6/ server type (IIS, Apache)
      7/ proxy servers - cloudfare, squid, etc.

      In order to test this we need to test all permutations in all supported browsers and for all themes.

      I know it is going to be a pain, but the perf benefits of this change could be huge on some sites.

      What to look for:

      • notices in PHP logs
      • missing files in apache log
      • errors in JS console
      • missing images
      • non-functional JS
      Show
      We need to test that we are not breaking file servering anywhere and that we are not ending up with broken links/resources, so there there are several parameters that affect static file serving and caching which will be need to be tried disabeld and enabled: 1/ $CFG->slasharguments 2/ $CFG->cachejs 3/ $CFG->themedesignermode 4/ $CFG->yuicomboloading 5/ ssl mode 6/ server type (IIS, Apache) 7/ proxy servers - cloudfare, squid, etc. In order to test this we need to test all permutations in all supported browsers and for all themes. I know it is going to be a pain, but the perf benefits of this change could be huge on some sites. What to look for: notices in PHP logs missing files in apache log errors in JS console missing images non-functional JS
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w19_MDL-32683_m23_publiccache

      Description

      https://developers.google.com/speed/docs/best-practices/caching

      1/ it should help FF to cache SSL
      2/ it should help proxy servers along the way
      3/ more files should be served from browser caches

      we could use it for: themes, yui, javascript, etc.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              skodak Petr Skoda added a comment -

              The patch might also resolve notices produced from the minifier and adds xsendfile support to lib/javascript.php.

              Show
              skodak Petr Skoda added a comment - The patch might also resolve notices produced from the minifier and adds xsendfile support to lib/javascript.php.
              Hide
              poltawski Dan Poltawski added a comment -

              Hi Petr,

              Could you explain a bit more what you've done in this change, its not clear from the issue title/description

              Show
              poltawski Dan Poltawski added a comment - Hi Petr, Could you explain a bit more what you've done in this change, its not clear from the issue title/description
              Hide
              skodak Petr Skoda added a comment - - edited

              The changes are described in https://developers.google.com/speed/docs/best-practices/caching:
              1/ it adds missing etags and other caching headers (to encourage as much caching as possible both in browsers and proxy caches)
              2/ it uses slash arguments for static files without cookies (? in url often prevents proxy caching)
              3/ it standardises lib/javascript.php serving (xsendfile, compression, no weird notices from minify caching, etc.)

              Show
              skodak Petr Skoda added a comment - - edited The changes are described in https://developers.google.com/speed/docs/best-practices/caching: 1/ it adds missing etags and other caching headers (to encourage as much caching as possible both in browsers and proxy caches) 2/ it uses slash arguments for static files without cookies (? in url often prevents proxy caching) 3/ it standardises lib/javascript.php serving (xsendfile, compression, no weird notices from minify caching, etc.)
              Hide
              poltawski Dan Poltawski added a comment -

              Hmm. interesting. I'm seeing a weird CSS issue after adding this. See screenshot.

              Show
              poltawski Dan Poltawski added a comment - Hmm. interesting. I'm seeing a weird CSS issue after adding this. See screenshot.
              Hide
              skodak Petr Skoda added a comment -

              oh, I forgot to mention you need to purge caches (it will be done automatically in upgrade), also we will have to force upgrade from main /index.php (but it is required for other changes introduced recently elsewhere). Please note you need to reset again if you use the same dataroot for checkouts without this patch.

              Show
              skodak Petr Skoda added a comment - oh, I forgot to mention you need to purge caches (it will be done automatically in upgrade), also we will have to force upgrade from main /index.php (but it is required for other changes introduced recently elsewhere). Please note you need to reset again if you use the same dataroot for checkouts without this patch.
              Hide
              skodak Petr Skoda added a comment -

              hmm, it might be better to bump up main version.php and force redirect_if_major_upgrade_required() right now and if necessary do it once again right before the release.

              Show
              skodak Petr Skoda added a comment - hmm, it might be better to bump up main version.php and force redirect_if_major_upgrade_required() right now and if necessary do it once again right before the release.
              Hide
              poltawski Dan Poltawski added a comment -

              I have reset caches a few times, still seeing this weird theme thing.

              Show
              poltawski Dan Poltawski added a comment - I have reset caches a few times, still seeing this weird theme thing.
              Hide
              skodak Petr Skoda added a comment -

              strange, what browser are you using?

              Show
              skodak Petr Skoda added a comment - strange, what browser are you using?
              Hide
              poltawski Dan Poltawski added a comment -

              Chrome (I wonder if its some CSS which was previously not properly loaded!)

              Show
              poltawski Dan Poltawski added a comment - Chrome (I wonder if its some CSS which was previously not properly loaded!)
              Hide
              skodak Petr Skoda added a comment -

              Chrome works fine here.

              Show
              skodak Petr Skoda added a comment - Chrome works fine here.
              Hide
              poltawski Dan Poltawski added a comment - - edited

              Confirming this change seems to be unrelated to the theme issues I was seeing. In fact it was from using a standardold in the course theme!

              Show
              poltawski Dan Poltawski added a comment - - edited Confirming this change seems to be unrelated to the theme issues I was seeing. In fact it was from using a standardold in the course theme!
              Hide
              poltawski Dan Poltawski added a comment -

              Just a note that the caching header mentioned is: 'cache-control: public'

              Show
              poltawski Dan Poltawski added a comment - Just a note that the caching header mentioned is: 'cache-control: public'
              Hide
              poltawski Dan Poltawski added a comment -

              OK, i've spent quite a bit of time playing around with this and think its looking good so i've integrated this, thanks!

              Show
              poltawski Dan Poltawski added a comment - OK, i've spent quite a bit of time playing around with this and think its looking good so i've integrated this, thanks!
              Hide
              poltawski Dan Poltawski added a comment -

              Grr!
              user_picture_testcase::test_get_url
              Failed asserting that two strings are equal.
              — Expected
              +++ Actual
              @@ @@
              -'http://www.example.com/moodle/theme/image.php?theme=standard&image=u%2Ff2&rev=1'
              +'http://www.example.com/moodle/theme/image.php/standard/core/1/u/f2'

              /Users/Shared/Jenkins/Home/git_repositories/master/lib/tests/outputcomponents_test.php:174
              /Users/Shared/Jenkins/Home/git_repositories/master/lib/phpunit/lib.php:1411

              Show
              poltawski Dan Poltawski added a comment - Grr! user_picture_testcase::test_get_url Failed asserting that two strings are equal. — Expected +++ Actual @@ @@ -'http://www.example.com/moodle/theme/image.php?theme=standard&image=u%2Ff2&rev=1' +'http://www.example.com/moodle/theme/image.php/standard/core/1/u/f2' /Users/Shared/Jenkins/Home/git_repositories/master/lib/tests/outputcomponents_test.php:174 /Users/Shared/Jenkins/Home/git_repositories/master/lib/phpunit/lib.php:1411
              Hide
              poltawski Dan Poltawski added a comment -

              Breaks the unit tests!

              Show
              poltawski Dan Poltawski added a comment - Breaks the unit tests!
              Hide
              skodak Petr Skoda added a comment -

              arrgh, I forgot that part, working on a fix right now

              Show
              skodak Petr Skoda added a comment - arrgh, I forgot that part, working on a fix right now
              Hide
              skodak Petr Skoda added a comment -

              fixes, sorry

              Show
              skodak Petr Skoda added a comment - fixes, sorry
              Hide
              poltawski Dan Poltawski added a comment -

              Integrated, thanks

              Show
              poltawski Dan Poltawski added a comment - Integrated, thanks
              Hide
              skodak Petr Skoda added a comment -

              it would be good to test it together with MDL-32825

              Show
              skodak Petr Skoda added a comment - it would be good to test it together with MDL-32825
              Hide
              skodak Petr Skoda added a comment -

              I have discovered a regression in lib/javascript.php "js_send_uncached($content, $etag);", it is fixed in the linked issue MDL-32825

              Show
              skodak Petr Skoda added a comment - I have discovered a regression in lib/javascript.php "js_send_uncached($content, $etag);", it is fixed in the linked issue MDL-32825
              Hide
              dobedobedoh Andrew Nicols added a comment -

              This is causing JS breakages where the component isn't specified as an arg to M.util.image_url. See MDL-32745 for an example.
              Have patch - just testing it.

              Show
              dobedobedoh Andrew Nicols added a comment - This is causing JS breakages where the component isn't specified as an arg to M.util.image_url. See MDL-32745 for an example. Have patch - just testing it.
              Hide
              skodak Petr Skoda added a comment -

              oh! thanks for spotting it

              Show
              skodak Petr Skoda added a comment - oh! thanks for spotting it
              Hide
              poltawski Dan Poltawski added a comment -

              Going to fail this test for the time being.

              Show
              poltawski Dan Poltawski added a comment - Going to fail this test for the time being.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              This should set a default component of 'core' now:

              git://git.luns.net.uk/
              MDL-32683-master-1
              https://git.luns.net.uk/moodle.git/commitdiff/b003b556cc7af38766de1d76fcff6f0f9d31bc73

              Show
              dobedobedoh Andrew Nicols added a comment - This should set a default component of 'core' now: git://git.luns.net.uk/ MDL-32683 -master-1 https://git.luns.net.uk/moodle.git/commitdiff/b003b556cc7af38766de1d76fcff6f0f9d31bc73
              Hide
              skodak Petr Skoda added a comment -

              thanks, looks ok! I am not sure which language I hate more, JS or PHP?

              Show
              skodak Petr Skoda added a comment - thanks, looks ok! I am not sure which language I hate more, JS or PHP?
              Hide
              skodak Petr Skoda added a comment - - edited

              sorry for the trouble, I hope any potential perf improvement will be worth all this.

              Show
              skodak Petr Skoda added a comment - - edited sorry for the trouble, I hope any potential perf improvement will be worth all this.
              Hide
              poltawski Dan Poltawski added a comment -

              Pushed that, thanks Andrew!

              Show
              poltawski Dan Poltawski added a comment - Pushed that, thanks Andrew!
              Hide
              skodak Petr Skoda added a comment -
              Show
              skodak Petr Skoda added a comment - one more sloppy regression - please merge https://github.com/skodak/moodle/commit/1c6d3af542b2945eac424b80da68efe359fc3d0b
              Show
              skodak Petr Skoda added a comment - and one more tweak https://github.com/skodak/moodle/commit/acc165aac781f5e51032dd10602d565bd50908b0
              Hide
              skodak Petr Skoda added a comment -

              both added to my original PULL request branch

              Show
              skodak Petr Skoda added a comment - both added to my original PULL request branch
              Hide
              poltawski Dan Poltawski added a comment -

              Pushed these fixes. Thanks

              Show
              poltawski Dan Poltawski added a comment - Pushed these fixes. Thanks
              Hide
              skodak Petr Skoda added a comment -

              I have created a new issue MDL-32849 for sites that use URL rewriting, because this patch might create problems for them (they would have to modify some of the old recommended regex rules).

              Show
              skodak Petr Skoda added a comment - I have created a new issue MDL-32849 for sites that use URL rewriting, because this patch might create problems for them (they would have to modify some of the old recommended regex rules).
              Hide
              abgreeve Adrian Greeve added a comment -

              I tested this using IE, Opera, Firefox and Chrome. It took a lot of time going through the different settings with the different themes in the different browsers. I'm not exactly sure how complete my testing was but some things that I did do were:

              • Add each of the different resources
              • Add each of the different activities
              • Change over an old assignment to a new one
              • upload images from my computer and repositories
              • Load up data presets
              • Upload a csv file
              • Enabled ajax and moved course elements around
              • created chat messages in the chat window
              • Editing settings and grades in the grade book
              • imported quiz questions into the question bank

              I didn't come across any missing images or non-functional JS.
              I got a lot of strict rule notices and warnings when ever I was using the file picker and uploading data to the site.

              I got a couple of javascript error messages while upgrading all of the old assignments to the new assignment type.

              Message: 'M.str.assign.noassignmentsselected' is null or not an object Line: 59

              Message: 'null' is null or not an object Line: 27

              both in admin/tool/assignmentupgrade/module.js

              Neither of these errors actually effect how the upgrade works. Everything still gets converted ok.

              I hope this helps.

              Show
              abgreeve Adrian Greeve added a comment - I tested this using IE, Opera, Firefox and Chrome. It took a lot of time going through the different settings with the different themes in the different browsers. I'm not exactly sure how complete my testing was but some things that I did do were: Add each of the different resources Add each of the different activities Change over an old assignment to a new one upload images from my computer and repositories Load up data presets Upload a csv file Enabled ajax and moved course elements around created chat messages in the chat window Editing settings and grades in the grade book imported quiz questions into the question bank I didn't come across any missing images or non-functional JS. I got a lot of strict rule notices and warnings when ever I was using the file picker and uploading data to the site. I got a couple of javascript error messages while upgrading all of the old assignments to the new assignment type. Message: 'M.str.assign.noassignmentsselected' is null or not an object Line: 59 Message: 'null' is null or not an object Line: 27 both in admin/tool/assignmentupgrade/module.js Neither of these errors actually effect how the upgrade works. Everything still gets converted ok. I hope this helps.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome?

              Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U

              Closing, ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome? Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U Closing, ciao

                People

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

                  Dates

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