Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.4
    • Component/s: Performance, Questions
    • Labels:
    • Testing Instructions:
      Hide

      You need to to a bunch of quiz and question things, and make sure there are no regressions:

      1. Preview a question.

      2. Create a quiz.

      3. Preview it.

      4. Attempt it as a student.

      5. As a teacher, view the quiz reports, and review some attempts.

      Show
      You need to to a bunch of quiz and question things, and make sure there are no regressions: 1. Preview a question. 2. Create a quiz. 3. Preview it. 4. Attempt it as a student. 5. As a teacher, view the quiz reports, and review some attempts.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      42794

      Description

      Please define the $cachecat variable and how it works. If it turns out to be something we can convert to MUC then we'll use this bug for it.

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          As the PHPdoc comment on question_has_capability_on says:

          • @param integer $cachecat useful to cache all question records in a category

          So, $cachecat does not cache anything, it tells the function to cache all the data from a particular question category with a couple of DB queries, to save DB queries on future calls to this function.

          The actual caching is done by

              static $questions = array();
              static $categories = array();
              static $cachedcat = array();
          

          The point is that to check permissions to do things to a question, you need to know question->createdby and question->category->contextid.


          That is really a trivial case. The more interesting question is

          • What sort of caching could questionlib be doing that it does not already do, in order to make the biggest difference?

          Loading question definitions

          Problem summary

          There is a significant un-solved problem: load_questions, or rather get_question_options which it calls.

          Typically, that is used any time someone is attempting or reviewing a quiz, and the $questions array is all the questions in the quiz, or at least on the current page of the quiz.

          get_question_options > _tidy_question -> question_bank::get_qtype($question>qtype)->get_question_options($question)

          and that get_question_options typically does a few DB queries.

          So, we are doing a loop which does a few DB queries per question in the body of the loop, which is a classic performance failure.

          Outline solution

          This has been a known problem for ages, but it is only recently that I realised that effectively this is a problem that we already solved in Moodle years ago.

          The place we have solved this is the modinfo cache for courses. Loading the key data for a module (e.g. name) takes several DB queries per module, so we cache the data in serialised form in course.modinfo.

          We can do exactly the same for question definitions, except that for question definitions, I would be inclined to do it slightly differently. For modules, we cache all the data for all the modules in a course in one entry in the course table.

          For questions, I think it is better to cache the data per-question, so we either add a column 'definitioninfo' (or something like that) to the question table, or, my preferred option at the moment, we add a new table question_definition_cache with columns id, questionid, and serialisedquestiondata. Anyway, that would hold the result of calling load_question in PHP serialised form.

          But obviously, even though I have thought that through, I am not planning to implement it until after I have seen how MUC goes.

          Show
          Tim Hunt added a comment - As the PHPdoc comment on question_has_capability_on says: @param integer $cachecat useful to cache all question records in a category So, $cachecat does not cache anything, it tells the function to cache all the data from a particular question category with a couple of DB queries, to save DB queries on future calls to this function. The actual caching is done by static $questions = array(); static $categories = array(); static $cachedcat = array(); The point is that to check permissions to do things to a question, you need to know question->createdby and question->category->contextid. That is really a trivial case. The more interesting question is What sort of caching could questionlib be doing that it does not already do, in order to make the biggest difference? Loading question definitions Problem summary There is a significant un-solved problem: load_questions, or rather get_question_options which it calls. Typically, that is used any time someone is attempting or reviewing a quiz, and the $questions array is all the questions in the quiz, or at least on the current page of the quiz. get_question_options > _tidy_question -> question_bank::get_qtype($question >qtype)->get_question_options($question) and that get_question_options typically does a few DB queries. So, we are doing a loop which does a few DB queries per question in the body of the loop, which is a classic performance failure. Outline solution This has been a known problem for ages, but it is only recently that I realised that effectively this is a problem that we already solved in Moodle years ago. The place we have solved this is the modinfo cache for courses. Loading the key data for a module (e.g. name) takes several DB queries per module, so we cache the data in serialised form in course.modinfo. We can do exactly the same for question definitions, except that for question definitions, I would be inclined to do it slightly differently. For modules, we cache all the data for all the modules in a course in one entry in the course table. For questions, I think it is better to cache the data per-question, so we either add a column 'definitioninfo' (or something like that) to the question table, or, my preferred option at the moment, we add a new table question_definition_cache with columns id, questionid, and serialisedquestiondata. Anyway, that would hold the result of calling load_question in PHP serialised form. But obviously, even though I have thought that through, I am not planning to implement it until after I have seen how MUC goes.
          Hide
          Tim Hunt added a comment -

          Assigning back to moodle.com, since I have provided the necessary information.

          Show
          Tim Hunt added a comment - Assigning back to moodle.com, since I have provided the necessary information.
          Hide
          moodle.com added a comment -

          After discussions, sounds like "Loading question definitions" could be a pretty easy win for 2.4.

          Show
          moodle.com added a comment - After discussions, sounds like "Loading question definitions" could be a pretty easy win for 2.4.
          Hide
          Tim Hunt added a comment -

          With a bit of help from Sam, I think this is working.

          Please note

          • You need to fix for MDL-36094 before this will work.
          • You need to go to the caches admin page, and click the "Reload definitions" link, since we have not bumped version.php yet.

          I think all the caching stuff is working. Before this is integrated, I should go through the rest of the question code, and convert all the other places that load questions to use the same mechanism, so they can benefit from the cache. This will let us deprecate some of the nasty old methods in questionlib.php.

          Since Sam helped me with this, it would be good of someone else could peer-review it.

          Show
          Tim Hunt added a comment - With a bit of help from Sam, I think this is working. Please note You need to fix for MDL-36094 before this will work. You need to go to the caches admin page, and click the "Reload definitions" link, since we have not bumped version.php yet. I think all the caching stuff is working. Before this is integrated, I should go through the rest of the question code, and convert all the other places that load questions to use the same mechanism, so they can benefit from the cache. This will let us deprecate some of the nasty old methods in questionlib.php. Since Sam helped me with this, it would be good of someone else could peer-review it.
          Hide
          Dan Poltawski added a comment -

          Sending all 'waiting for peer review' issues to integration before freeze, as agreed in Integrators Meeting 19/10/12. We are doing this to ensure any 'integratable issues' will not got missed before freeze..

          Show
          Dan Poltawski added a comment - Sending all 'waiting for peer review' issues to integration before freeze, as agreed in Integrators Meeting 19/10/12. We are doing this to ensure any 'integratable issues' will not got missed before freeze..
          Hide
          Tim Hunt added a comment -

          I have done a bit of worthwhile clean-up. It is not quite everything I think should be cleaned up, but it is a useful place to stop, and get this integrated, before I go on holiday for a week.

          Anyway, the point of this comment is to say that this is now good to be integrated.

          Show
          Tim Hunt added a comment - I have done a bit of worthwhile clean-up. It is not quite everything I think should be cleaned up, but it is a useful place to stop, and get this integrated, before I go on holiday for a week. Anyway, the point of this comment is to say that this is now good to be integrated.
          Hide
          Dan Poltawski added a comment -

          Integrated to master, thanks Tim!

          Show
          Dan Poltawski added a comment - Integrated to master, thanks Tim!
          Hide
          Dan Poltawski added a comment -

          Doh, I need to bump version.php

          Show
          Dan Poltawski added a comment - Doh, I need to bump version.php
          Hide
          Dan Poltawski added a comment -

          Eek.

          I went to an existing quiz:

          This question is missing. Unable to display anything.

          Show
          Dan Poltawski added a comment - Eek. I went to an existing quiz: This question is missing. Unable to display anything.
          Hide
          Dan Poltawski added a comment -

          Ah, I think the problem was MDL-36094

          Show
          Dan Poltawski added a comment - Ah, I think the problem was MDL-36094
          Hide
          Jason Fowler added a comment -

          all fine Tim, thanks for the fix

          Show
          Jason Fowler added a comment - all fine Tim, thanks for the fix
          Hide
          Dan Poltawski added a comment -

          Hurray!

          You did it, congratulations! You have on Mojito credit to redeem after the release of Moodle 2.4

          Show
          Dan Poltawski added a comment - Hurray! You did it, congratulations! You have on Mojito credit to redeem after the release of Moodle 2.4
          Hide
          Martin Dougiamas added a comment -

          (Mojito credits redeemable only at Moodle Pty Ltd premises during business hours in the 30 solar days after the release of Moodle 2.4. Credits are non-transferable. Mojitos may contain variable ingredients and previous samples should not be taken as a guarantee of future Mojito quality. Mojitos may cause blindness and/or diarrhea)

          Show
          Martin Dougiamas added a comment - (Mojito credits redeemable only at Moodle Pty Ltd premises during business hours in the 30 solar days after the release of Moodle 2.4. Credits are non-transferable. Mojitos may contain variable ingredients and previous samples should not be taken as a guarantee of future Mojito quality. Mojitos may cause blindness and/or diarrhea)

            People

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

              Dates

              • Created:
                Updated:
                Resolved: