Moodle
  1. Moodle
  2. MDL-36761

Unnecessary calls to "count()" can be optimized to improve performance

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.6, 2.3.3, 2.4
    • Fix Version/s: 2.2.7, 2.3.4, 2.4.1
    • Component/s: Gradebook
    • Labels:
    • Testing Instructions:
      Hide

      This code is pretty conclusively covered by the unit tests (phpunit grade_category_testcase lib/grade/tests/grade_category_test.php)

      Show
      This code is pretty conclusively covered by the unit tests (phpunit grade_category_testcase lib/grade/tests/grade_category_test.php)
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-36761_loop
    • Rank:
      46273

      Description

      In the function "apply_limit_rules()" in "/lib/grade/grade_category.php", the "count()" function is called repeatedly on a variable that does not change. For large data sets, this can cause an performance hit.
      Consider changing so that count is called only once:
      From:

      $i = 1;
      while ($originalindex+$i < count($grade_keys)) {
          $possibleitemid = $grade_keys[$originalindex+$i];
          $i++;
      ...
      

      To:

      $i = 1;
      $gradekeycount = count($grade_keys);
      while ($originalindex+$i < $gradekeycount) {
          $possibleitemid = $grade_keys[$originalindex+$i];
          $i++;
      ...
      

        Activity

        Hide
        Andrew Davis added a comment -

        Adding a git branch containing the suggested improvement.

        Show
        Andrew Davis added a comment - Adding a git branch containing the suggested improvement.
        Hide
        Ankit Agarwal added a comment -

        Hi Andrew,
        Will it be better to move it even up and declare it near L888

                        $grade_keys = array_keys($grade_values);
                        if (count($grade_keys) === 0) {
                            //We've dropped all grade items
                            break;
                        }
        

        Rest looks good. Feel free to ignore this suggestion and submit for integration.
        Thanks

        Show
        Ankit Agarwal added a comment - Hi Andrew, Will it be better to move it even up and declare it near L888 $grade_keys = array_keys($grade_values); if (count($grade_keys) === 0) { //We've dropped all grade items break ; } Rest looks good. Feel free to ignore this suggestion and submit for integration. Thanks
        Hide
        Andrew Davis added a comment -

        Well spotted Ankit. I've done as you suggested.

        Added 2.3 and 2.2 versions. Putting this up for integration.

        Show
        Andrew Davis added a comment - Well spotted Ankit. I've done as you suggested. Added 2.3 and 2.2 versions. Putting this up for integration.
        Hide
        Sam Hemelryk added a comment -

        Thanks Andrew, this has been integrated now.

        Show
        Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now.
        Hide
        Frédéric Massart added a comment -

        Test passed on 2.2, 2.3 and 2.4. Thanks.

        For 2.2, I have set a new category to drop 1 lowest grade, then graded students on 2 activities. The lowest grade was ignored without any errors.

        Show
        Frédéric Massart added a comment - Test passed on 2.2, 2.3 and 2.4. Thanks. For 2.2, I have set a new category to drop 1 lowest grade, then graded students on 2 activities. The lowest grade was ignored without any errors.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now!

        Closing, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now! Closing, ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: