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

assignsubmission_comments: Improve performance for permissions checking

    Details

    • Testing Instructions:
      Hide

      1) Open a assignment Grading Table with over 50 rows with submissions and comments per page.

      2) Verify web server and database performance metrics and page load time.

      3) Apply the fix.

      4) Verify performance improvement for the grading table.

      5)
      5) a) Suspend a student (Suspend the enrolment not the student account)
      5) b) Log in as a tutor without capability 'moodle/course:viewsuspendedusers'
      5) c) Should not be able to view the suspended user in the assignment grading table.

      Show
      1) Open a assignment Grading Table with over 50 rows with submissions and comments per page. 2) Verify web server and database performance metrics and page load time. 3) Apply the fix. 4) Verify performance improvement for the grading table. 5) 5) a) Suspend a student (Suspend the enrolment not the student account) 5) b) Log in as a tutor without capability 'moodle/course:viewsuspendedusers' 5) c) Should not be able to view the suspended user in the assignment grading table.
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE, MOODLE_27_STABLE
    • Pull 2.7 Branch:
      MDL-45678-006-27
    • Pull Master Branch:
      MDL-45678-006-master

      Description

      When viewing the grading table, a permissions callback will check for suspended users for each submission in the table.

      Please view the attached callgraph alongside my explanation regarding my changes.

      In assign_submission_comments::view_summary(), there is a call to the comment constructor. This constructor calls comment::check_permissions(), which is expensive – for each row!

      According to the callgraph, the reason why comment::check_permissions() is expensive, is not because it calls has_capability(), but rather because it calls assignsubmission_comments_comment_permissions().

      If you view assignsubmission_comments_comment_permissions(), it checks get_suspended_userids(), which generates an expensive SQL query in pg_query_params().

      My fix is to cache the result of get_suspended_userids() in a MODE_REQUEST Cache.

        Gliffy Diagrams

          Attachments

          1. callgraph.png
            callgraph.png
            4.38 MB
          2. callgraph.svg
            231 kB
          3. callgrind.out
            566 kB
          4. grading-screen.png
            grading-screen.png
            213 kB
          5. grading-screen-master27.png
            grading-screen-master27.png
            170 kB

            Issue Links

              Activity

                People

                • Votes:
                  2 Vote for this issue
                  Watchers:
                  9 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    14/Jul/14