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

          Attachments

            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: