Moodle
  1. Moodle
  2. MDL-17556

Performance with large courses is very poor eg get_fast_modinfo isn't

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.9.3
    • Fix Version/s: 1.9.4
    • Component/s: Libraries
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      36654

      Description

      Performance of get_fast_modinfo is poor when courses have many activities, especially on Postgres.

      This is because the main query get_fast_modinfo does is to retrieve the list of contexts and it does this with an 'instanceid IN (...)' syntax. This syntax is slow when retrieving large number of results. We observed this on a genuine course with 900 activities. (Yes this is special purpose but still.)

      Performance can be improved by using a standard join rather than trying to 'optimise' the database query by passing in the already-known instance IDs. I have done a patch to make this happen.

      Here are some tests on dev system (note, Postgres/MySQL times are different servers so not comparable, but tests on the same db are on the same machine; all queries were run repeatedly 5 times in a row after at least one/two warmups):

      Postgres (before, after)

      2000 activities: ~1500ms, ~140ms
      ~30 activities: ~30ms, ~30ms

      MySQL (before, after)

      2000 activities: ~500ms, ~170ms
      3 activities: ~20ms, ~20ms

      So in other words there is no difference in performance for small courses and for very large courses this is an improvement of roughly 10x on the performance of get_fast_modinfo in Postgres, or roughly 3x in MySQL.

      1. getfastmodinfo.core.1.patch
        3 kB
        Sam Marshall
      2. getfastmodinfo.core.2.patch
        4 kB
        Sam Marshall

        Activity

        Hide
        Sam Marshall added a comment -

        Could somebody review this short patch? If nobody objects I will get it reviewed locally then commit it for 1.9.4 and 2.x.

        (Since it is a worthwhile performance improvement imo - ok not many courses have 900 activities, that's a bit of a special case, but we have 25 courses with more than 200...)

        Show
        Sam Marshall added a comment - Could somebody review this short patch? If nobody objects I will get it reviewed locally then commit it for 1.9.4 and 2.x. (Since it is a worthwhile performance improvement imo - ok not many courses have 900 activities, that's a bit of a special case, but we have 25 courses with more than 200...)
        Hide
        Petr Škoda added a comment -

        wow 900!
        going to review it

        Show
        Petr Škoda added a comment - wow 900! going to review it
        Hide
        Petr Škoda added a comment -

        hmm, wouldn't it be easier to add new function that preloads all course related contexts including blocks?

        Show
        Petr Škoda added a comment - hmm, wouldn't it be easier to add new function that preloads all course related contexts including blocks?
        Hide
        Tim Hunt added a comment -

        I agree with Petr, a separate function

        get_all_activity_contexts($courseid);

        would made the intent of the code much clearer, and move the SQL out of course/lib.php and into accesslib.php.

        Show
        Tim Hunt added a comment - I agree with Petr, a separate function get_all_activity_contexts($courseid); would made the intent of the code much clearer, and move the SQL out of course/lib.php and into accesslib.php.
        Hide
        Sam Marshall added a comment -

        ARGH ***ING BROWSER LOST THIS TEXT WHEN I PRESSED THE WRONG KEY *tries to calm down

        Revised patch attached. Gets rid of other changes, makes new function preload_course_contexts which loads (UNION ALL) course context, module contexts, and block contexts (for blocks on course page). Performance seems similar. Changed get_fast_modinfo to call this, also threw in a call at the top of course/view to save a query loading the course context.

        OK to commit this for 1.9? (Or of course feel free to make more comments...)

        I may have other changes later - still working on other performance issues with ridiculous number of activities - but figure best to deal with them one at a time.

        Show
        Sam Marshall added a comment - ARGH *** ING BROWSER LOST THIS TEXT WHEN I PRESSED THE WRONG KEY *tries to calm down Revised patch attached. Gets rid of other changes, makes new function preload_course_contexts which loads (UNION ALL) course context, module contexts, and block contexts (for blocks on course page). Performance seems similar. Changed get_fast_modinfo to call this, also threw in a call at the top of course/view to save a query loading the course context. OK to commit this for 1.9? (Or of course feel free to make more comments...) I may have other changes later - still working on other performance issues with ridiculous number of activities - but figure best to deal with them one at a time.
        Hide
        Sam Marshall added a comment -

        I committed this patch to MOODLE_19.

        I'll merge it to HEAD and tag _MERGED later today (need to get other changes committed first).

        Show
        Sam Marshall added a comment - I committed this patch to MOODLE_19. I'll merge it to HEAD and tag _MERGED later today (need to get other changes committed first).
        Hide
        Sam Marshall added a comment -

        OK, now also committed to HEAD, and tagged (on the branch) MOODLE_19_MERGED. I tested only briefly in HEAD in a 'does the course page still load' kind of way, but that's probably adequate.

        Show
        Sam Marshall added a comment - OK, now also committed to HEAD, and tagged (on the branch) MOODLE_19_MERGED. I tested only briefly in HEAD in a 'does the course page still load' kind of way, but that's probably adequate.
        Hide
        Petr Škoda added a comment -

        reopening, the use of whitespace is not acceptable, sorry - please see http://docs.moodle.org/en/Development:Coding
        going to fix it now...

        Show
        Petr Škoda added a comment - reopening, the use of whitespace is not acceptable, sorry - please see http://docs.moodle.org/en/Development:Coding going to fix it now...
        Hide
        Petr Škoda added a comment -

        reclosing, thanks!

        Show
        Petr Škoda added a comment - reclosing, thanks!
        Hide
        Tim Hunt added a comment -

        Sam, was there a good reason why you did a union of three queries, rather than just

        SELECT * FROM

        {context}

        WHERE path LIKE '$coursecontext->path/%'

        (I think course context will already be cached, if not, query can be adjusted.)

        Show
        Tim Hunt added a comment - Sam, was there a good reason why you did a union of three queries, rather than just SELECT * FROM {context} WHERE path LIKE '$coursecontext->path/%' (I think course context will already be cached, if not, query can be adjusted.)
        Hide
        Sam Marshall added a comment -

        Tim - LIKE queries are not indexed in UTF-8 Postgres unless you add extra index; that would perform far worse on a standard Postgres install.

        (There is a bug about this for Eloy, but I want to go home not look up the number, sorry!)

        So at the point when I wrote this, probably the unions were faster. (We do have the special index manually added on OU servers now, for obvious reasons.)

        If Eloy adds support for xmldb to make the indexes, this would probably only happen in 2.0. Once that happens, yes it would be worth re-evaluating performance of the simple LIKE query against the union, on mysql and postgres - it might perform better [or just the same and with simpler code].

        Show
        Sam Marshall added a comment - Tim - LIKE queries are not indexed in UTF-8 Postgres unless you add extra index; that would perform far worse on a standard Postgres install. (There is a bug about this for Eloy, but I want to go home not look up the number, sorry!) So at the point when I wrote this, probably the unions were faster. (We do have the special index manually added on OU servers now, for obvious reasons.) If Eloy adds support for xmldb to make the indexes, this would probably only happen in 2.0. Once that happens, yes it would be worth re-evaluating performance of the simple LIKE query against the union, on mysql and postgres - it might perform better [or just the same and with simpler code] .

          People

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

            Dates

            • Created:
              Updated:
              Resolved: