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

block_glossary issues (performance and caching)

    Details

    • Affected Branches:
      MOODLE_30_STABLE, MOODLE_31_STABLE

      Description

      Whilst reviewing MDL-53598, I've found multiple issues with the random glossary block.

      These are largely performance-related issues, but some other issues are also present.

      At the moment I am combining these into a single issue as I suspect that they will be best solved by a refactor.

      Initial setup
      1. Install another language (e.g. German)
      2. Enable the multilang filter at all levels
      3. Create a new glossary
      4. Add an entry which makes use of multilang:
          <span lang="en" class="multilang">A small(ish) horse</span>
          <span lang="de" class="multilang">ein großes Kaninchen</span>
        
      5. Add an instance of the block to the page and configure it to use this glossary - all other settings default.
      Observations
      1. With the block on the page there are now 5 new DB reads, and 2 new DB writes:
        // In specialization function:
        SELECT COUNT('x') FROM mdl_glossary_entries WHERE glossaryid = $1 AND approved = $2 [array ( 0 => '1', 1 => 1, )]
        SELECT * FROM mdl_glossary WHERE id = $1 [array ( 0 => '1', )]
        SELECT cm.*, m.name, md.name AS modname FROM mdl_course_modules cm JOIN mdl_modules md ON md.id = cm.module JOIN mdl_glossary m ON m.id = cm.instance WHERE m.id = $1 AND md.name = $2 AND cm.course = $3 [array ( 0 => '1', 1 => 'glossary', 2 => '7', )]
        SELECT id, concept, definition, definitionformat, definitiontrust FROM mdl_glossary_entries WHERE glossaryid = $1 AND approved = 1 ORDER BY timemodified ASC LIMIT 1 OFFSET 0 [array ( 0 => '1', )]
        UPDATE mdl_block_instances SET configdata = $2 WHERE id = $1 [array ( 0 => '45', 1 => 'Tzo4OiJzdGRDbGFzcyI6MTM6e3M6NToidGl0bGUiO3M6MjE6IlJhbmRvbSBnbG9zc2FyeSBlbnRyeSI7czo4OiJnbG9zc2FyeSI7czoxOiIxIjtzOjc6InJlZnJlc2giO2k6MDtzOjQ6InR5cGUiO3M6MToiMCI7czoxMToic2hvd2NvbmNlcHQiO3M6MToiMSI7czo4OiJhZGRlbnRyeSI7czoxNToiQWRkIGEgbmV3IGVudHJ5IjtzOjEyOiJ2aWV3Z2xvc3NhcnkiO3M6MTY6IlZpZXcgYWxsIGVudHJpZXMiO3M6OToiaW52aXNpYmxlIjtzOjE3OiIodG8gYmUgY29udGludWVkKSI7czo4OiJuZXh0dGltZSI7aToxNDY0MTA1NjAwO3M6NToiY2FjaGUiO3M6MTc1OiI8aDM+RG9nSMO8bmQ8L2gzPjxkaXYgY2xhc3M9Im5vLW92ZXJmbG93Ij48c3BhbiBsYW5nPSJlbiIgY2xhc3M9Im11bHRpbGFuZyI+QSBzbWFsbChpc2gpIGhvcnNlPC9zcGFuPg0KDQo8c3BhbiBsYW5nPSJkZSIgY2xhc3M9Im11bHRpbGFuZyI+RWluZSBncsO2c3NlIGthbmluc2NoZW48L3NwYW4+PC9kaXY+IjtzOjE0OiJnbG9iYWxnbG9zc2FyeSI7czoxOiIwIjtzOjg6ImNvdXJzZWlkIjtzOjE6IjciO3M6ODoicHJldmlvdXMiO2k6MTt9', )]
        
        // In get_content function:
        SELECT * FROM mdl_glossary WHERE id = $1 [array ( 0 => '1', )]
        UPDATE mdl_block_instances SET configdata = $2 WHERE id = $1 [array ( 0 => '45', 1 => 'Tzo4OiJzdGRDbGFzcyI6MTM6e3M6NToidGl0bGUiO3M6MjE6IlJhbmRvbSBnbG9zc2FyeSBlbnRyeSI7czo4OiJnbG9zc2FyeSI7czoxOiIxIjtzOjc6InJlZnJlc2giO2k6MDtzOjQ6InR5cGUiO3M6MToiMCI7czoxMToic2hvd2NvbmNlcHQiO3M6MToiMSI7czo4OiJhZGRlbnRyeSI7czoxNToiQWRkIGEgbmV3IGVudHJ5IjtzOjEyOiJ2aWV3Z2xvc3NhcnkiO3M6MTY6IlZpZXcgYWxsIGVudHJpZXMiO3M6OToiaW52aXNpYmxlIjtzOjE3OiIodG8gYmUgY29udGludWVkKSI7czo4OiJuZXh0dGltZSI7aToxNDY0MTA1NjAwO3M6NToiY2FjaGUiO3M6MTc1OiI8aDM+RG9nSMO8bmQ8L2gzPjxkaXYgY2xhc3M9Im5vLW92ZXJmbG93Ij48c3BhbiBsYW5nPSJlbiIgY2xhc3M9Im11bHRpbGFuZyI+QSBzbWFsbChpc2gpIGhvcnNlPC9zcGFuPg0KDQo8c3BhbiBsYW5nPSJkZSIgY2xhc3M9Im11bHRpbGFuZyI+RWluZSBncsO2c3NlIGthbmluc2NoZW48L3NwYW4+PC9kaXY+IjtzOjE0OiJnbG9iYWxnbG9zc2FyeSI7czoxOiIwIjtzOjg6ImNvdXJzZWlkIjtzOjE6IjciO3M6ODoicHJldmlvdXMiO2k6MTt9', )]
        

        The specialization function fetches the glossary record from the table, but does not cache it internally, even though it is almost always fetched within the same request.

      2. We write block configuration whenever the block is viewed. This usually happens twice (once in specialization and once in get_content). This also happens regardless of whether the configuration has changed
      Subsequent stuff
      1. Modify the block configuraiton to set the value of Days before a new entry is chosen to 1.
      2. View the page with the block again
      3. Note the displayed entry
      4. Open the glossary and modify that entry (e.g. correct a typo)
      5. Refresh the page with the block again
        1. The original content is still shown
      6. Delete the entry altogether
        1. The original content is still shown
      7. Change page language to German and Refresh the page with the block again
        1. The original content is still shown in English
      8. Modify the configuration and reset the Days value to 0
      9. Refresh the page with the block again
        1. The original content is still shown in English
      Recommendations

      From what I can see, the specialization function is being misused to pre-fetch some information.
      As a result some values are fetched in the specialization function, and also in get_content.

      A good first step would be to standardise the fetching of configuration and content into a single place and then to remove the specialization function if it is not required.
      We should also remove the excessive config writes (modify to write changes only on data change), and switch from get_coursemodule_from_instance to get_fast_modinfo.
      Following on from this we need to fix the issues with the caching of entries in their filtered and formatted state - the best way of doing this is probably by caching the entry id rather than the end result. It should not be a cache but a current record holder. If caching is desired, MUC should be used to support invalidation.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

                People

                • Assignee:
                  Unassigned
                  Reporter:
                  dobedobedoh Andrew Nicols
                  Participants:
                  Component watchers:
                  Adrian Greeve, Mihail Geshoski, Jake Dallimore, Jun Pataleta, Adrian Greeve, Mihail Geshoski, Matteo Scaramuccia, Ankit Agarwal, David Monllaó
                • Votes:
                  1 Vote for this issue
                  Watchers:
                  2 Start watching this issue

                  Dates

                  • Created:
                    Updated: