Moodle
  1. Moodle
  2. MDL-40266

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

    Details

    • Rank:
      51034

      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.

        Issue Links

          Activity

          Hide
          Tim Gus added a comment -

          xhprof as run for a SCORM report

          Show
          Tim Gus added a comment - xhprof as run for a SCORM report
          Hide
          Petr Škoda added a comment -

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

          Show
          Petr Škoda added a comment - hello, the change looks ok, but why do you open second connection in the unit test?
          Hide
          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
          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
          Petr Škoda added a comment -

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

          Show
          Petr Škoda added a comment - You can extend the class with protected method, add new method there that gives you access to the protected method.
          Hide
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          Petr Škoda added a comment -

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

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

          patches for ms drivers submitted in MDL-40399

          Show
          Petr Škoda added a comment - patches for ms drivers submitted in MDL-40399
          Hide
          Petr Škoda added a comment -

          Why did you reopen this?

          Show
          Petr Škoda added a comment - Why did you reopen this?
          Hide
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          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
          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 Agarwal added a comment -

          passing as unit tests are run during integration

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

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

          Cheers, Damyon

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