Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Critical 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:
    • Rank:
      50104

      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.

      1. after.png
        3.80 MB
      2. before.png
        4.19 MB

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          Does this seem like a good change?

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

          Before and after profiling attached.

          Show
          Tim Hunt added a comment - Before and after profiling attached.
          Hide
          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 Mudrak in case he has some objections to this.

          Show
          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 Mudrak in case he has some objections to this.
          Hide
          Dan Poltawski added a comment -

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

          Show
          Dan Poltawski added a comment - ps. Can you add testing instructions, making sure to test with debug_developer off..
          Hide
          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
          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
          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
          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
          Dan Poltawski added a comment -

          Yep, agreed - feed us impressive numbers

          Show
          Dan Poltawski added a comment - Yep, agreed - feed us impressive numbers
          Hide
          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
          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
          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
          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
          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
          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
          David Mudrak added a comment -

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

          Show
          David Mudrak added a comment - +1 for the suggested solution, thanks guys for working on this.
          Hide
          Tim Hunt added a comment -

          Thanks everyone, submitting for integration.

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

          Integrated (23, 24 & master), thanks!

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

          (unit tests passed)

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

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

          So passing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - (visited course/activities view/edit pages, admin pages... everything looking ok, with debugging disabled) So passing, thanks!
          Hide
          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
          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: