Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 2.3.6, 2.4.3, 2.5
    • Fix Version/s: 2.3.7, 2.4.4
    • Component/s: Performance
    • Labels:
    • Testing Instructions:
      Hide

      1. Run all the unit tests.
      2. Poke around you Moodle site with developer debug off, and verify nothing is obviously broken.

      Show
      1. Run all the unit tests. 2. Poke around you Moodle site with developer debug off, and verify nothing is obviously broken.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      This seems to be a regression caused by MDL-22015.

      The clean_param is useful for developers, but we should not be paying that price in live use.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              timhunt Tim Hunt added a comment -

              Does this seem like a good change?

              Show
              timhunt Tim Hunt added a comment - Does this seem like a good change?
              Hide
              timhunt Tim Hunt added a comment -

              Before and after profiling attached.

              Show
              timhunt Tim Hunt added a comment - Before and after profiling attached.
              Hide
              poltawski Dan Poltawski added a comment -

              Hi Tim,

              I like the change. I agree the string components are not user input so we should not be cleaning them on every request. If there are cases where the string is coming from a user then we should be validating at that point, not when passed to get_string.

              The last line is quite long, but I see that is in keeping with the existing code.

              Pinging David Mudrák in case he has some objections to this.

              Show
              poltawski Dan Poltawski added a comment - Hi Tim, I like the change. I agree the string components are not user input so we should not be cleaning them on every request. If there are cases where the string is coming from a user then we should be validating at that point, not when passed to get_string. The last line is quite long, but I see that is in keeping with the existing code. Pinging David Mudrák in case he has some objections to this.
              Hide
              poltawski Dan Poltawski added a comment -

              ps. Can you add testing instructions, making sure to test with debug_developer off..

              Show
              poltawski Dan Poltawski added a comment - ps. Can you add testing instructions, making sure to test with debug_developer off..
              Hide
              poltawski Dan Poltawski added a comment -

              Oh and also I found those profiling images impossible to interpret - why does clean_param become yellow in the after profile and not the before) I can see there are half the number of calls to it.

              Show
              poltawski Dan Poltawski added a comment - Oh and also I found those profiling images impossible to interpret - why does clean_param become yellow in the after profile and not the before) I can see there are half the number of calls to it.
              Hide
              quen Sam Marshall added a comment -

              I agree the images are impossible to interpret without spending hours on it. I couldn't even find the right box as there is no search feature (doesn't also help that it is really hard to scroll around large images in browser)! Tim had some actual numbers, I think if you believe the profiler, this change reduces total page load time by 10% or something? It would be useful to include these in the issue so as to indicate how critical it is...

              Show
              quen Sam Marshall added a comment - I agree the images are impossible to interpret without spending hours on it. I couldn't even find the right box as there is no search feature (doesn't also help that it is really hard to scroll around large images in browser)! Tim had some actual numbers, I think if you believe the profiler, this change reduces total page load time by 10% or something? It would be useful to include these in the issue so as to indicate how critical it is...
              Hide
              poltawski Dan Poltawski added a comment -

              Yep, agreed - feed us impressive numbers

              Show
              poltawski Dan Poltawski added a comment - Yep, agreed - feed us impressive numbers
              Hide
              timhunt Tim Hunt added a comment -

              Testing instructions done.

              You guy's suck . It did not take me hours to make sense of those diagrams. Each box is a function, and it shows what calls what. The important number is the %age inside the box, which is how much of the page load time was caused by that function. The numbers on the lines are the number of call.

              So, wehn you see thousands of calls to clean_param, and that is taking 5% of the page-load time, you think that is stupid, and fix it. Then you look to see that your change has really made the clean_param percentage significantly smaller.

              Because the images are so big, you really need to save them to your desktop, and then drag them back into the browser. Then it is easer to scroll around.

              Show
              timhunt Tim Hunt added a comment - Testing instructions done. You guy's suck . It did not take me hours to make sense of those diagrams. Each box is a function, and it shows what calls what. The important number is the %age inside the box, which is how much of the page load time was caused by that function. The numbers on the lines are the number of call. So, wehn you see thousands of calls to clean_param, and that is taking 5% of the page-load time, you think that is stupid, and fix it. Then you look to see that your change has really made the clean_param percentage significantly smaller. Because the images are so big, you really need to save them to your desktop, and then drag them back into the browser. Then it is easer to scroll around.
              Hide
              timhunt Tim Hunt added a comment -

              Oh, and the colours attempt to hilight the most significant parts for you to look at, but they are no always helpful, and they don't always reveal the easiest low-hanging fruit.

              So, in sumamry:

              Before: 1290 calls to clean_param, 5% of page-load time.
              After: 747 calls to clean_param, 3% of page-load time.

              So, about a 2% overall speed-up from this simple change.

              Show
              timhunt Tim Hunt added a comment - Oh, and the colours attempt to hilight the most significant parts for you to look at, but they are no always helpful, and they don't always reveal the easiest low-hanging fruit. So, in sumamry: Before: 1290 calls to clean_param, 5% of page-load time. After: 747 calls to clean_param, 3% of page-load time. So, about a 2% overall speed-up from this simple change.
              Hide
              quen Sam Marshall added a comment -

              Aww, only 2%? Still, better than a kick in the teeth.

              I find it impossible to scroll large images in browser at all because it makes you use scroll bars, which are utterly unusable for images. Would have to save into a program like gimp or something, that has a draggy hand feature (I have no idea why firefox doesn't do that... maybe chrome does, but my chrome doesn't appear to be working at the moment, so).

              Anyway, point being, they're maybe useful if you want to make a detailed consideration of all the possible performance problems on a page but they're a really terrible way to illustrate performance changes for one difficult-to-find function

              Show
              quen Sam Marshall added a comment - Aww, only 2%? Still, better than a kick in the teeth. I find it impossible to scroll large images in browser at all because it makes you use scroll bars, which are utterly unusable for images. Would have to save into a program like gimp or something, that has a draggy hand feature (I have no idea why firefox doesn't do that... maybe chrome does, but my chrome doesn't appear to be working at the moment, so). Anyway, point being, they're maybe useful if you want to make a detailed consideration of all the possible performance problems on a page but they're a really terrible way to illustrate performance changes for one difficult-to-find function
              Hide
              mudrd8mz David Mudrák added a comment -

              +1 for the suggested solution, thanks guys for working on this.

              Show
              mudrd8mz David Mudrák added a comment - +1 for the suggested solution, thanks guys for working on this.
              Hide
              timhunt Tim Hunt added a comment -

              Thanks everyone, submitting for integration.

              Show
              timhunt Tim Hunt added a comment - Thanks everyone, submitting for integration.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Integrated (23, 24 & master), thanks!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (23, 24 & master), thanks!
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              (unit tests passed)

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - (unit tests passed)
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              (visited course/activities view/edit pages, admin pages... everything looking ok, with debugging disabled)

              So passing, thanks!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - (visited course/activities view/edit pages, admin pages... everything looking ok, with debugging disabled) So passing, thanks!
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks! You're changes are now spread to the world through this git and our source control repositories.

              No time to rest though, we've got days to make 2.5 the best yet!

              ciao

              Show
              poltawski Dan Poltawski added a comment - Thanks! You're changes are now spread to the world through this git and our source control repositories. No time to rest though, we've got days to make 2.5 the best yet! ciao

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    13/May/13