Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-39443 META: OU Moodle 2.4 performance tuning
  3. MDL-39473

cache::parse_key: supports_multiple_identifiers check a bit slow

    Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.3
    • Fix Version/s: 2.4.4
    • Component/s: Caching
    • Labels:
      None

      Description

      The cache::parse_key function takes a fair chunk of time (in my test page it is called 482 times). As part of the MUC infrastructure this should be optimised.

      I didn't find very much wrong with it, but a small detail is that the supports_multiple_identifiers check takes a significant amount of time. It is possible to optimise this (albeit with some duplication) by overriding the function in each of the cache store types for which it always has a fixed value anyway. (I left the mongodb cache alone, because that one's more complicated.)

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            quen Sam Marshall added a comment -

            I profiled this change twice using the best-of-three method (3 profiles 'before' immediately followed by 3 profiles 'after', compare best of, using wall time for function parse_key):

            21.7ms -> 17.5ms
            21.7ms -> 17.3ms

            So about 4ms. Probably worthwhile as it is a rather safe change and I don't think the duplication is really too horrible...

            Show
            quen Sam Marshall added a comment - I profiled this change twice using the best-of-three method (3 profiles 'before' immediately followed by 3 profiles 'after', compare best of, using wall time for function parse_key): 21.7ms -> 17.5ms 21.7ms -> 17.3ms So about 4ms. Probably worthwhile as it is a rather safe change and I don't think the duplication is really too horrible...
            Hide
            timhunt Tim Hunt added a comment -

            This really needs peer review from Sam H, but +1 from me.

            Show
            timhunt Tim Hunt added a comment - This really needs peer review from Sam H, but +1 from me.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Absolutely. A good change thanks Sam, I've put this up for integration review immediately.

            Show
            samhemelryk Sam Hemelryk added a comment - Absolutely. A good change thanks Sam, I've put this up for integration review immediately.
            Hide
            poltawski Dan Poltawski added a comment -

            Integrated to master, thanks Sam

            Show
            poltawski Dan Poltawski added a comment - Integrated to master, thanks Sam
            Hide
            poltawski Dan Poltawski added a comment -

            Unit tests all good.

            Show
            poltawski Dan Poltawski added a comment - Unit tests all good.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Did you think this day was not going to arrive ever?

            Your patience has been rewarded, yay, sent upstream, thanks!

            Closing...ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Did you think this day was not going to arrive ever? Your patience has been rewarded, yay, sent upstream, thanks! Closing...ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  13/May/13