Moodle

cache::parse_key: supports_multiple_identifiers check a bit slow

Details

  • Type: Sub-task Sub-task
  • Status: Closed Closed
  • Priority: Minor 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.)

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
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
Permalink
Tim Hunt added a comment -

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

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

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

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

Integrated to master, thanks Sam

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

Unit tests all good.

Show
Dan Poltawski added a comment - Unit tests all good.
Hide
Permalink
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
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

Vote (1)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: