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

General performance fix for slow bound query parameter evaluations in emulate_bound_params in mysqli

    Details

      Description

      This issue was first discovered with the SCORM reports having slow page load times and the fix turned out to be a general performance increase for Moodle. We have about 40,000 Moodle users and our basic and interactions SCORM reports span hundreds of pages.

      It turns out that the call to array_shift in function emulate_bound_params from the file lib/dml/mysqli_native_moodle_database.php is very slow when it needs to evaluate thousands of bound query parameters.
      The function array_shift reindexes the array which is not necessary for the function emulate_bound_params, and is at least one reason for the bad performance.

      Our fix involves using array_reverse and array_pop for faster bound parameter evaluations. These are the before and after stats for a SCORM report page load time:
      Pre-patch page load time: 128.496265 secs
      Post-patch page load time: 5.149125 secs

      We only made the fix for mysqli.

      Please take a look at these branches at http://github.com/timgus/moodle for the patch and unit tests (for M23 and up):
      array_shift_fix_m22
      array_shift_fix_m23
      array_shift_fix_m24
      array_shift_fix_m25
      array_shift_fix_master

      I've attached my xhprof run before and after the fix when viewing a SCORM report page.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            tgus Tim Gus added a comment -

            xhprof as run for a SCORM report

            Show
            tgus Tim Gus added a comment - xhprof as run for a SCORM report
            Hide
            skodak Petr Skoda added a comment -

            hello, the change looks ok, but why do you open second connection in the unit test?

            Show
            skodak Petr Skoda added a comment - hello, the change looks ok, but why do you open second connection in the unit test?
            Hide
            tgus Tim Gus added a comment -

            Since emulate_bound_params is protected I don't have direct access to it so I can't do $DB->emulate_bound_params using the standard global $DB

            Show
            tgus Tim Gus added a comment - Since emulate_bound_params is protected I don't have direct access to it so I can't do $DB->emulate_bound_params using the standard global $DB
            Hide
            skodak Petr Skoda added a comment -

            You can extend the class with protected method, add new method there that gives you access to the protected method.

            Show
            skodak Petr Skoda added a comment - You can extend the class with protected method, add new method there that gives you access to the protected method.
            Hide
            martinlanghoff Martín Langhoff added a comment -

            The fix patch is mine, so biased +1 from my corner – it is in production @RL.

            We tried several approaches to fixing up the loop, including index lookups through $parts ($parts[$i]) and replacing the $result concats in the loop with an array ($result[] = $foo in the loop, then implode('', $result).

            This patch was the fastest implementation by a good margin.

            I don't grok the unit tests

            Show
            martinlanghoff Martín Langhoff added a comment - The fix patch is mine, so biased +1 from my corner – it is in production @RL. We tried several approaches to fixing up the loop, including index lookups through $parts ($parts [$i] ) and replacing the $result concats in the loop with an array ($result[] = $foo in the loop, then implode('', $result). This patch was the fastest implementation by a good margin. I don't grok the unit tests
            Hide
            skodak Petr Skoda added a comment -

            Cool, I will look at the tests tomorrow. In any case the rest of DML/DDL tests should prove if it works or not reliably. I will make sure it gets to integration next week.

            Show
            skodak Petr Skoda added a comment - Cool, I will look at the tests tomorrow. In any case the rest of DML/DDL tests should prove if it works or not reliably. I will make sure it gets to integration next week.
            Hide
            skodak Petr Skoda added a comment -

            Hello,

            I have cherry picked only the first commit and amended the commit message to match our recommended commit message style. I believe it is not necessary to add extra unit test for this fix because it is tested indirectly by most other DML tests - any problems would be immediately discovered during phpunit execution of all testcases.

            Thanks a lot for your report and patch, it was all my fault, I already fixed one other place with incorrect use of array_shift() - this was a very valuable lesson for me.

            Moodle 2.3 general support ends today, only critical or security patches will be accepted until December. I hope my rebase is not going to cause problems for your project. See http://docs.moodle.org/dev/Commit_cheat_sheet (Please note that the "code area:" part is considered useless by some developers, I would not recommend that.)

            Ciao

            Show
            skodak Petr Skoda added a comment - Hello, I have cherry picked only the first commit and amended the commit message to match our recommended commit message style. I believe it is not necessary to add extra unit test for this fix because it is tested indirectly by most other DML tests - any problems would be immediately discovered during phpunit execution of all testcases. Thanks a lot for your report and patch, it was all my fault, I already fixed one other place with incorrect use of array_shift() - this was a very valuable lesson for me. Moodle 2.3 general support ends today, only critical or security patches will be accepted until December. I hope my rebase is not going to cause problems for your project. See http://docs.moodle.org/dev/Commit_cheat_sheet (Please note that the "code area:" part is considered useless by some developers, I would not recommend that.) Ciao
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Petr,

            It looks like we can apply this same improvement to the mssql and sqlsrv drivers too. I don't see why we would only apply it to one of the three drivers using this 'hack'.

            Show
            poltawski Dan Poltawski added a comment - Hi Petr, It looks like we can apply this same improvement to the mssql and sqlsrv drivers too. I don't see why we would only apply it to one of the three drivers using this 'hack'.
            Hide
            skodak Petr Skoda added a comment -

            Yes Dan, but that would be a separate issue - creating...

            Show
            skodak Petr Skoda added a comment - Yes Dan, but that would be a separate issue - creating...
            Hide
            skodak Petr Skoda added a comment -

            patches for ms drivers submitted in MDL-40399

            Show
            skodak Petr Skoda added a comment - patches for ms drivers submitted in MDL-40399
            Hide
            skodak Petr Skoda added a comment -

            Why did you reopen this?

            Show
            skodak Petr Skoda added a comment - Why did you reopen this?
            Hide
            poltawski Dan Poltawski added a comment -

            Because you gave a partial solution, and now you've manipulated the issue into suggesting you gave a full issue. We could've avoided all this bureaucracy quite easily, and I don't agree this was a seperate issue.

            Show
            poltawski Dan Poltawski added a comment - Because you gave a partial solution, and now you've manipulated the issue into suggesting you gave a full issue. We could've avoided all this bureaucracy quite easily, and I don't agree this was a seperate issue.
            Hide
            skodak Petr Skoda added a comment -

            reopening would mean that this does not get to 2.5.1, also if anything failed in mssql (which I can not test right now) you would have problems with revert...

            Show
            skodak Petr Skoda added a comment - reopening would mean that this does not get to 2.5.1, also if anything failed in mssql (which I can not test right now) you would have problems with revert...
            Hide
            poltawski Dan Poltawski added a comment -

            Integrated to master, 25, 24 and cherry-picked to 23 (since this is still in bugfix support until next week)..

            thanks all.
            Dan

            Show
            poltawski Dan Poltawski added a comment - Integrated to master, 25, 24 and cherry-picked to 23 (since this is still in bugfix support until next week).. thanks all. Dan
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            passing as unit tests are run during integration

            Show
            ankit_frenz Ankit Agarwal added a comment - passing as unit tests are run during integration
            Hide
            damyon Damyon Wiese added a comment -

            This issue is fixed! Hurray! Hurray!
            Your issue is fixed, what a wonderful day!

            Cheers, Damyon

            Show
            damyon Damyon Wiese added a comment - This issue is fixed! Hurray! Hurray! Your issue is fixed, what a wonderful day! Cheers, Damyon

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  8/Jul/13