Terrible performance in the course format code

Marina, please could you take a look at this.

Performance change looking a a moderately sized course page as student:

Before: 135 calls to format_base::get_format_or_default, 8.5% of the page-load time.

After: 135 calls to format_base::get_format_or_default, 0.7% of the page-load time.

Tested with V2.5.0.3 of Collapsed Topics on a 52 section course and the page load is around 1 second quicker on rough average. Using FireBug.

Nice patch - this is the kind of thing that is hard to pick up on a dev site - but on a real site chock full of data really helps.

Maybe it could be even faster still:

$plugins = get_sorted_course_formats();
if (in_array($format, $plugins)) {
+ self::$classesforformat[$format] = $format;

Why not cache all the $plugins so get_sorted_course_formats only gets called once?

Hi!
Thanks for noticing it Tim.
Do I understand correctly that get_sorted_course_formats() is causing problems?
If yes, I would better do caching inside it.

Damyon, good idea, I will amend that patch.

Also, this was really easy to pick up on a dev site. That is where I found it. You just had to look at the profiling data.

Marina, actually, I did that first, then changed it. Think about what the two functions are doing:

1. format_base::get_format_or_default -> given then name of a selected course format from the DB, decide which real course format to instantiate.

2. get_sorted_course_formats -> get the list of all installed course formats, in order, which involves scanning a directory, and applying a lot of unnecessary configuration.

Really, there is no need to be using 2. to do 1. I guess that was just a quick and easy way to write the code. 1. is the think that needs to be fast, so it was easiest to put the static cache there.

1. should really just check the one requested course format. It should not load all of them. If you did move the caching back into 2. then 1. would continue to load the list of all course formats every time, which is silly. So, anyway, I think the caching belongs in 1.

Hi Tim,
ok. I really think that the checking availability and enable-ability is the slow point, not sorting. But since this function is not used often except here this is up to you to decide.

which reminds me of MDL-35727 where I plan to get rid of static cache...

Commits amended based on Damyon's comments. Submitting for integration now.

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

The patch looks fine Tim, but I couldn't get that warning to display using those instructions - I think you might even have to do some database twiddling to get that to happen. They are not important to the patch because it looks to have the same behaviour as before so I have removed those last 2 parts form the instructions.

Damyon, it was my testing instruction. Yes probably one needs to physically remove format from disc to get the warning

Yes, I was going to say, thanks Maria for extending the testing instructions.

Thanks Tim, this has been integrated to 2.4 and master.

Its difficult to be entirely sure but, as far as I can tell, this is working fine. Passing.

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

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

Closing...ciao