Details

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

      Description

      I am seeing 600 calls even when when logged in as student, and it hurts.

      It turns out that a separate is_valid_plugin_name function is a win.

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

        Activity

        Hide
        Tim Hunt added a comment -

        Petr, I'm asking you to peer review, because this seems like your sort of thing. Hoever, if you don't want it, please un-assign yourself. Thanks.

        Show
        Tim Hunt added a comment - Petr, I'm asking you to peer review, because this seems like your sort of thing. Hoever, if you don't want it, please un-assign yourself. Thanks.
        Hide
        Tim Hunt added a comment -

        Here are before and after profiling results. Before, clean_param ~= 3% of page load. Afterwards, it is nowhere (so not in the list of the top 100 functions).

        Show
        Tim Hunt added a comment - Here are before and after profiling results. Before, clean_param ~= 3% of page load. Afterwards, it is nowhere (so not in the list of the top 100 functions).
        Hide
        Tim Hunt added a comment -

        Performance data:

        Before: 747 calls to clean_param, 3% page-load time;

        After: 84 calls to clean_param, 0.5% page-load time; 629 calls to is_valid_plugin_name, 0.8% page-load time

        So, about a 1.5% to 2% speed-up.

        Master commit amended to include unit tests. (I will re-cherry-pick once everyone is happy.)

        Show
        Tim Hunt added a comment - Performance data: Before: 747 calls to clean_param, 3% page-load time; After: 84 calls to clean_param, 0.5% page-load time; 629 calls to is_valid_plugin_name, 0.8% page-load time So, about a 1.5% to 2% speed-up. Master commit amended to include unit tests. (I will re-cherry-pick once everyone is happy.)
        Hide
        Eloy Lafuente (stronk7) added a comment -

        While I like the speed-up... why is there such difference? Does the switch() introduce it? or the is_array() and is_object() at the beginning?

        I mean, if any of those are the "culprits" for the slowdown... then all the clean_param() calls are damn slow. All.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - While I like the speed-up... why is there such difference? Does the switch() introduce it? or the is_array() and is_object() at the beginning? I mean, if any of those are the "culprits" for the slowdown... then all the clean_param() calls are damn slow. All. Ciao
        Hide
        Dan Poltawski added a comment -

        You('re tests) suck!

        function is_valid_plugin_name($name) {
        $this->assertFalse((bool) is_valid_plugin_name('xx_', PARAM_PLUGIN), '');
        
        Show
        Dan Poltawski added a comment - You('re tests) suck! function is_valid_plugin_name($name) { $ this ->assertFalse((bool) is_valid_plugin_name('xx_', PARAM_PLUGIN), '');
        Hide
        Dan Poltawski added a comment -

        Hi Tim,

        Seems pragmatic solution to me. It would be good to share in this issue responses to Eloy in why clean_param wasn't 'fixable'.

        Dan

        Show
        Dan Poltawski added a comment - Hi Tim, Seems pragmatic solution to me. It would be good to share in this issue responses to Eloy in why clean_param wasn't 'fixable'. Dan
        Hide
        Tim Hunt added a comment -

        Eloy, That is a good question. There were a variety of reasons:

        1. Each of is_array, is_object and switch add a small amount of overheard. They really should not, but given the number of times we call this function, it adds up to something significant.

        2. Also, I read a bit about PHP performance (e.g. http://stackoverflow.com/questions/3399755/if-versus-switch). PHP compiles switch into a big if/else (but a bit slower). Therefore, I hoped it woudl be enough just to move PARAM_PLUGIN to be the first thing in the if, but disppointingly that made almost no difference.

        So, a separate function was much faster in my testing. So the above Performance data. (Sorry I did not record the numbers for the re-order the switch option.)

        Show
        Tim Hunt added a comment - Eloy, That is a good question. There were a variety of reasons: 1. Each of is_array, is_object and switch add a small amount of overheard. They really should not, but given the number of times we call this function, it adds up to something significant. 2. Also, I read a bit about PHP performance (e.g. http://stackoverflow.com/questions/3399755/if-versus-switch ). PHP compiles switch into a big if/else (but a bit slower). Therefore, I hoped it woudl be enough just to move PARAM_PLUGIN to be the first thing in the if, but disppointingly that made almost no difference. So, a separate function was much faster in my testing. So the above Performance data. (Sorry I did not record the numbers for the re-order the switch option.)
        Hide
        Tim Hunt added a comment -

        All branches amended. The 2.3 version touches fewer files because there is no cache code there.

        Submitting for integration now.

        Show
        Tim Hunt added a comment - All branches amended. The 2.3 version touches fewer files because there is no cache code there. Submitting for integration now.
        Hide
        Dan Poltawski 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
        Dan Poltawski 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
        Damyon Wiese added a comment -

        Didn't get to finish testing it - but I profiled the change too and got:

        Profiling on master I get:
        Without patch:
        clean_param 3.29 3.06 (3781)

        With patch:

        clean_param 2.87 2.66 (3467)
        is_valid_plugin_name 0.06, 0.04 (314)

        I'll stop the review for now

        Show
        Damyon Wiese added a comment - Didn't get to finish testing it - but I profiled the change too and got: Profiling on master I get: Without patch: clean_param 3.29 3.06 (3781) With patch: clean_param 2.87 2.66 (3467) is_valid_plugin_name 0.06, 0.04 (314) I'll stop the review for now
        Hide
        Tim Hunt added a comment -

        I don't understand:

        1. What docs dev docs you think are necessary, besides the PHPdoc I already wrote.

        2. What it means that you have stopped integration review.

        3. Are you testing with developer debug off? I expect the huge number of clean_param calls come from get_string, which checks for valid string names if developer debug is on.

        Show
        Tim Hunt added a comment - I don't understand: 1. What docs dev docs you think are necessary, besides the PHPdoc I already wrote. 2. What it means that you have stopped integration review. 3. Are you testing with developer debug off? I expect the huge number of clean_param calls come from get_string, which checks for valid string names if developer debug is on.
        Hide
        Damyon Wiese added a comment - - edited

        Hi Tim,

        Stopping review just means - that it was Friday and I wasn't going to finished reviewing/integrating before I left work so I put it back in the queue in case someone else wanted to grab it over the weekend. I was posting my profiling just to backup your data (they show the same improvement). (Sorry for the confusion)

        Show
        Damyon Wiese added a comment - - edited Hi Tim, Stopping review just means - that it was Friday and I wasn't going to finished reviewing/integrating before I left work so I put it back in the queue in case someone else wanted to grab it over the weekend. I was posting my profiling just to backup your data (they show the same improvement). (Sorry for the confusion)
        Hide
        Damyon Wiese added a comment -

        Some comments - I don't like adding a new function to the API for PARAM_PLUGIN - it is used alot and with this patch we will be calling it in 2 different ways in core/non-core old/new code.

        Some more testing results from this patch:

        (These are averages of 4 runs - each)

        % time in clean_param + is_valid_plugin_name combined

        No changes:
        3.2075%

        Just the modified regex change:
        3.2425%

        Full patch (separate function for is_valid_plugin_name)
        2.9875%

        Separate if at the top of clean_param
        2.9275%

        The last one is the result of this code change:

        function clean_param($param, $type) {
        
            global $CFG;
        
            // This is separated out because it is performance critical.                         
            if ($type == PARAM_PLUGIN) {
                if (!preg_match('/^[a-z](?:[a-z0-9_](?!__))*[a-z0-9]$/', $param)) {
                    return '';
                }
                return $param;
            }
        

        Which is IMO a better result - no api change and the same performance improvement.

        Show
        Damyon Wiese added a comment - Some comments - I don't like adding a new function to the API for PARAM_PLUGIN - it is used alot and with this patch we will be calling it in 2 different ways in core/non-core old/new code. Some more testing results from this patch: (These are averages of 4 runs - each) % time in clean_param + is_valid_plugin_name combined No changes: 3.2075% Just the modified regex change: 3.2425% Full patch (separate function for is_valid_plugin_name) 2.9875% Separate if at the top of clean_param 2.9275% The last one is the result of this code change: function clean_param($param, $type) { global $CFG; // This is separated out because it is performance critical. if ($type == PARAM_PLUGIN) { if (!preg_match('/^[a-z](?:[a-z0-9_](?!__))*[a-z0-9]$/', $param)) { return ''; } return $param; } Which is IMO a better result - no api change and the same performance improvement.
        Hide
        Tim Hunt added a comment -

        Damyon, I think that is a worse API change, because now clean_param has an inconsistent API. For PARAM_...anything else, you have certain behaviour if an array or object is passed in. If PARAM_PLUGIN is passed in, something different happens.

        In addition, if you look at the two places where I changed the call to clean_param, you will see that that code really did not want to clean a plugin name at all. It was doing

        if (clean_param(...) !== '') {

        In other words, it wanted to know if a plugin name was valid, and asked that question by cleaning the name, and seeing if that made it blank. An is_valid function is a much more natural API there.

        In other places, like on plugin management pages, where we are doing optional_ or require_param, then clean_param is the natural API. Of course, to avoid duplicated code, in that case clean_param calls is_valid_plugin_name.

        So, I think your proposal is worse than mine, but it might be a matter of taste.

        Show
        Tim Hunt added a comment - Damyon, I think that is a worse API change, because now clean_param has an inconsistent API. For PARAM_...anything else, you have certain behaviour if an array or object is passed in. If PARAM_PLUGIN is passed in, something different happens. In addition, if you look at the two places where I changed the call to clean_param, you will see that that code really did not want to clean a plugin name at all. It was doing if (clean_param(...) !== '') { In other words, it wanted to know if a plugin name was valid, and asked that question by cleaning the name, and seeing if that made it blank. An is_valid function is a much more natural API there. In other places, like on plugin management pages, where we are doing optional_ or require_param, then clean_param is the natural API. Of course, to avoid duplicated code, in that case clean_param calls is_valid_plugin_name. So, I think your proposal is worse than mine, but it might be a matter of taste.
        Hide
        Damyon Wiese added a comment -

        Thanks Tim,

        I was swayed by your convincing arguments (the main one being that the calling functions were not using the output anyway).

        This has been integrated to 23, 24 and master branches - I've tested it alot already and will pass the test once CI agrees.

        Show
        Damyon Wiese added a comment - Thanks Tim, I was swayed by your convincing arguments (the main one being that the calling functions were not using the output anyway). This has been integrated to 23, 24 and master branches - I've tested it alot already and will pass the test once CI agrees.
        Hide
        Tim Hunt added a comment -

        Thanks Damyon.

        Show
        Tim Hunt added a comment - Thanks Damyon.
        Hide
        Damyon Wiese added a comment -

        Tests passed in CI.

        Show
        Damyon Wiese added a comment - Tests passed in CI.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Did you think this day was not going to arrive ever?

        Your patience has been rewarded, yay, sent upstream, thanks!

        Closing...ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Did you think this day was not going to arrive ever? Your patience has been rewarded, yay, sent upstream, thanks! Closing...ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: