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

            dobedobedoh Andrew Nicols created issue -
            dobedobedoh Andrew Nicols made changes -
            Field Original Value New Value
            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:
            The first time the editor is displayed, on my default 2.1 install I see 57 queries
            For each subsequent time the editor is displayed, I see a further 54 queries.
            On the multichoice question type, there are 17 editors, so 57 + (54 * 16) = 921 queries

            Enabling an additional repository plugin (URL downloader) changes these numbers
            The first time the editor is displayed: 69 queries
            For each subsequent time the editor is displayed, I see a further 66 queries.
            On the multichoice question type, there are 17 editors, so 69 + (66 * 16) = 1125 queries

            (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.
            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:
            {code}
            First editor: 57 queries
            Subsequent editors: 54 queries each
            Total queries for 17 editors = 57 + (54 * 16) = 921
            {code}

            Enabling an additional repository plugin (URL downloader) changes these numbers
            {code}
            First editor: 69 queries
            Subsequent editors: 66 queries each
            Total queries for 17 editors = 69 + (66 * 16) = 1125
            {code}
            (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:
            {code}
            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', )]
            {code}

            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.
            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.
            dobedobedoh Andrew Nicols made changes -
            Pull Master Diff URL https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=a05902999478ffe705e8a86bf9924fa1575638a8
            Pull Master Branch MDL-29831-master
            Pull from Repository git://git.luns.net.uk/moodle
            Labels editor performance
            Difficulty Moderate
            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.
            poltawski Dan Poltawski made changes -
            Priority Minor [ 4 ] Critical [ 2 ]
            Component/s Performance [ 10221 ]
            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.
            poltawski Dan Poltawski made changes -
            Labels editor performance editor performance triaged
            poltawski Dan Poltawski made changes -
            Fix Version/s STABLE backlog [ 10463 ]
            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
            poltawski Dan Poltawski made changes -
            Link This issue will be resolved by MDL-25290 [ MDL-25290 ]
            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.
            marina Marina Glancy made changes -
            Link This issue has a non-specific relationship to MDL-34346 [ MDL-34346 ]
            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.
            salvetore Michael de Raadt made changes -
            Link This issue will be resolved by MDL-36228 [ MDL-36228 ]
            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] ; +
            marina Marina Glancy made changes -
            Link This issue has a non-specific relationship to MDL-34346 [ MDL-34346 ]
            marina Marina Glancy made changes -
            Link This issue will be (partly) resolved by MDL-34346 [ MDL-34346 ]
            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
            marina Marina Glancy made changes -
            Status Open [ 1 ] Closed [ 6 ]
            Resolution Duplicate [ 3 ]

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: