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

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

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: 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

      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++;
      ...

        Gliffy Diagrams

          Activity

          Hide
          andyjdavis Andrew Davis added a comment -

          Adding a git branch containing the suggested improvement.

          Show
          andyjdavis Andrew Davis added a comment - Adding a git branch containing the suggested improvement.
          Hide
          ankit_frenz 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_frenz 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
          andyjdavis 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
          andyjdavis 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
          samhemelryk Sam Hemelryk added a comment -

          Thanks Andrew, this has been integrated now.

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now.
          Hide
          fred 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
          fred 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
          stronk7 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
          stronk7 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:
                Fix Release Date:
                14/Jan/13