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

Multiple occurrences of the editor on the same page cause excessive db queries

    Details

    • Database:
      Any
    • Testing Instructions:
      Hide

      Enable perf to footer for ease
      From Home as administrator:

      • Settings -> Front Page Settings -> Questions
      • Select 'Create a new Question'
      • Choose 'Multiple Choice'
      • Note number of queries
      • Refresh page
      • Note number of queries
      • From the breadcrumbs, choose 'Edit question'
      • Select 'Create a new Question'
      • Choose 'True/False'
      • Note number of queries
      • Refresh page
      • Note number of queries

      Enable an additional Repository plugin (I chose URL Downloader with default settings)
      Repeat

      Show
      Enable perf to footer for ease From Home as administrator: Settings -> Front Page Settings -> Questions Select 'Create a new Question' Choose 'Multiple Choice' Note number of queries Refresh page Note number of queries From the breadcrumbs, choose 'Edit question' Select 'Create a new Question' Choose 'True/False' Note number of queries Refresh page Note number of queries Enable an additional Repository plugin (I chose URL Downloader with default settings) Repeat
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-29831-master

      Description

      On a page with multiple copies of the editor (e.g. when adding a new multichoice question), an excessive number of database reads are seen.

      When the editor is inserted into a page, lib/form/editor.php::MoodleQuickForm_editor->toHtml() is called
      This in turn calls initialise_filepicker once for each type of media plugin - e.g. advimage, moodlemedia, and advlink
      The initialise_filepicker() function in turn calls repository::get_instances which generates a set of queries for each of the repository plugins.

      As an example on my default 2.1 install:

      First editor: 57 queries
      Subsequent editors: 54 queries each
      Total queries for 17 editors = 57 + (54 * 16) = 921

      Enabling an additional repository plugin (URL downloader) changes these numbers

      First editor: 69 queries
      Subsequent editors: 66 queries each
      Total queries for 17 editors = 69 + (66 * 16) = 1125

      (Note, I loaded the page twice after adding enabling additional repository plugins)

      We should see if it's possible to cache the results for some of these queries. They're mostly of the nature:

      SELECT * FROM mdl_repository_instances WHERE id = $1 [array ( 0 => '1', )]
      SELECT * FROM mdl_repository_instance_config WHERE instanceid = $1 [array ( 0 => '1', )]
      SELECT * FROM mdl_repository WHERE id = $1 [array ( 0 => '1', )]
      SELECT name,value FROM mdl_config_plugins WHERE plugin = $1 [array ( 0 => 'local', )]

      This is seen for any user (not just administrators). The question component is not at fault, but provides a handy set of editors to try.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            poltawski Dan Poltawski added a comment -

            Adding Tim as a watcher here as he may have come across it

            Show
            poltawski Dan Poltawski added a comment - Adding Tim as a watcher here as he may have come across it
            Hide
            timhunt Tim Hunt added a comment -

            Ouch! I had not noticed that.

            Sounds solvable with some simple caching in repository class.

            Show
            timhunt Tim Hunt added a comment - Ouch! I had not noticed that. Sounds solvable with some simple caching in repository class.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I've identified the key culprits and added some caching.

            The main one is the get_type_by_id() in is_visible. Caching this saves two queries per repository per media plugin, per editor.

            Caching the instance config in the repository constructor saved another two per repo, per media plugin, per editor.

            This took me down to just over 300 reads so there's still a significant amount of caching that can be done.

            Show
            dobedobedoh Andrew Nicols added a comment - I've identified the key culprits and added some caching. The main one is the get_type_by_id() in is_visible. Caching this saves two queries per repository per media plugin, per editor. Caching the instance config in the repository constructor saved another two per repo, per media plugin, per editor. This took me down to just over 300 reads so there's still a significant amount of caching that can be done.
            Hide
            poltawski Dan Poltawski added a comment -

            This is a real issue. It is making editing multichoice questions practically unusable for me, with the perf stats from produciton site:

            time: 66.553539s memory_total: 28413856B (27.1MB) memory_growth: 27638344B (26.4MB) memory_peak: 30130256B (28.7MB) includecount: 501 contextswithfilters: 0 filterscreated: 0 textsfiltered: 0 stringsfiltered: 0 langcountgetstring: 3216 langcountmemcache: 3041 langcountdiskcache: 183 includedyuimodules: 54 includedjsmodules: 4 db reads/writes: 4338/2 ticks: 6656 user: 291 sys: 35 cuser: 0 csys: 0 serverload: 0.00 Session: 57.9KB

            Show
            poltawski Dan Poltawski added a comment - This is a real issue. It is making editing multichoice questions practically unusable for me, with the perf stats from produciton site: time: 66.553539s memory_total: 28413856B (27.1MB) memory_growth: 27638344B (26.4MB) memory_peak: 30130256B (28.7MB) includecount: 501 contextswithfilters: 0 filterscreated: 0 textsfiltered: 0 stringsfiltered: 0 langcountgetstring: 3216 langcountmemcache: 3041 langcountdiskcache: 183 includedyuimodules: 54 includedjsmodules: 4 db reads/writes: 4338/2 ticks: 6656 user: 291 sys: 35 cuser: 0 csys: 0 serverload: 0.00 Session: 57.9KB
            Hide
            poltawski Dan Poltawski added a comment -

            Upgrading priority and adding performance component.

            Show
            poltawski Dan Poltawski added a comment - Upgrading priority and adding performance component.
            Hide
            poltawski Dan Poltawski added a comment -

            Urgh. I think more work than this needs to be done to fix it up properly.

            For example we are already retrieving the data which you are caching in the calling code

            Show
            poltawski Dan Poltawski added a comment - Urgh. I think more work than this needs to be done to fix it up properly. For example we are already retrieving the data which you are caching in the calling code
            Hide
            poltawski Dan Poltawski added a comment -

            There seem to be 36 calls to repository::get_instances() on that page

            Show
            poltawski Dan Poltawski added a comment - There seem to be 36 calls to repository::get_instances() on that page
            Hide
            dobedobedoh Andrew Nicols added a comment - - edited

            IIRC, there are three TinyMCE plugins which use repositories. get_instances is called for each of these, and for each time the editor appears on screen.

            I'd guess that the page you're looking at has 12 TinyMCE editors on it, with three TinyMCE plugins each.

            Show
            dobedobedoh Andrew Nicols added a comment - - edited IIRC, there are three TinyMCE plugins which use repositories. get_instances is called for each of these, and for each time the editor appears on screen. I'd guess that the page you're looking at has 12 TinyMCE editors on it, with three TinyMCE plugins each.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Not knowing how complex it is... but I think the only correct solution to this is to delay the calculation of available repository instances to the exact moment they are requested (aka, press the add image/media... buttons, when the popup is shown).

            Anything else will require tricky caching, surely not covering all the situations.

            My 2 cents, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Not knowing how complex it is... but I think the only correct solution to this is to delay the calculation of available repository instances to the exact moment they are requested (aka, press the add image/media... buttons, when the popup is shown). Anything else will require tricky caching, surely not covering all the situations. My 2 cents, ciao
            Hide
            tlevi Tony Levi added a comment -

            A couple of simple static caches work great for this problem.

            Show
            tlevi Tony Levi added a comment - A couple of simple static caches work great for this problem.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Tony,

            You'll notice that this is the suggestion I made before, but there is an issue with this kind of static caching – although you may have multiple editors on a single page, they may have different repositories available to them.
            Catering for this can be difficult.

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Tony, You'll notice that this is the suggestion I made before, but there is an issue with this kind of static caching – although you may have multiple editors on a single page, they may have different repositories available to them. Catering for this can be difficult. Andrew
            Hide
            poltawski Dan Poltawski added a comment -

            Andrew, you made a comment about this in dev chat. We are working on MUC (MDL-25290) for 2.4, please don't make any progress on this until we know where we are going with MUC as its a prime candidate.

            Show
            poltawski Dan Poltawski added a comment - Andrew, you made a comment about this in dev chat. We are working on MUC ( MDL-25290 ) for 2.4, please don't make any progress on this until we know where we are going with MUC as its a prime candidate.
            Hide
            tlevi Tony Levi added a comment -

            Well, for interest sake, I found this in git which I think was related to this problem but it might be evil:

            function initialise_filepicker($args) {
            global $CFG, $USER, $PAGE, $OUTPUT;
            static $templatesinitialized;
            + static $cache = array();
            +
            + $key = serialize($args).$USER->id;
            + if (!empty($cache[$key])) return $cache[$key];
            +

            Show
            tlevi Tony Levi added a comment - Well, for interest sake, I found this in git which I think was related to this problem but it might be evil: function initialise_filepicker($args) { global $CFG, $USER, $PAGE, $OUTPUT; static $templatesinitialized; + static $cache = array(); + + $key = serialize($args).$USER->id; + if (!empty($cache [$key] )) return $cache [$key] ; +
            Hide
            marina Marina Glancy added a comment -

            This issue is resolved under MDL-34346

            Show
            marina Marina Glancy added a comment - This issue is resolved under MDL-34346

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: