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:
    • 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

        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: