justinhunt / moodle-filter_poodll

The PoodLL Filter
6 stars 17 forks source link

Only read local_config once #32

Closed aolley closed 5 years ago

aolley commented 5 years ago

Without this change, filter_poodll is performing lookups for config far more than necessary. This change reduces it to one read total per page view.

On any given page, it should only need to load the course config once. However without this patch we actually see:

course with no set poodll config:

Where an "invocation" is a call to the filter - so for every label and such on the page. We're seeing an average of 50 DB reads for the same data in course views as a result.

aolley commented 5 years ago

Basically, if a course doesnt have any custom config set for poodll, $this->courseconfig is set to an empty array - which evaluates to false, so the next time the function is called it tries to load the (absent) data again.

justinhunt commented 5 years ago

Thanks Adam. Appreciate you looking at this. It looks fine, but I wonder shouldn't we set $this->courseconfig to the value of $data, in the case that it was able to pull it from cache ... like this ..

if ($data === false) { $this->courseconfig = filter_get_local_config('poodll', context_course::instance($COURSE->id)->id); $cache->set($contextid, $this->courseconfig); }else{ $this->courseconfig = $data; }

aolley commented 5 years ago

Uh, yes. Yes we should. Thanks for picking that up.

justinhunt commented 5 years ago

Thanks @aolley thats merged now. Appreciate it.

justinhunt commented 5 years ago

@aolley If there are any more performance issues related to this site, can you let me know? I have been talking with some of their team in another thread. justin at poodll

aolley commented 5 years ago

@justinhunt this was the only poodll related issue I identified.