Perf fixes#10723
Conversation
|
Where are the tests? |
| @stage = @script_level.stage | ||
|
|
||
| if current_user | ||
| # Precache all user_levels for this script to avoid duplicate database dips over the course of generating this page. |
There was a problem hiding this comment.
nit: The controller method has no notion of "this page".
nit: The DB dips would not be "duplicate", right? Previously, these dips were for different levels? Perhaps "to bulk query".
| # result. | ||
| user_levels.find_by(script_id: script_level.script_id, | ||
| level_id: level.id) | ||
| level_id: level.id) |
There was a problem hiding this comment.
nit: If you are adjusting the whitespace, any reason not to combine onto previous line?
| # clear the per-request cache when user levels are saved or updated | ||
| def clear_user_level_cache | ||
| user.clear_user_level_cache unless user.nil | ||
| end |
There was a problem hiding this comment.
This method seems to not exist.
Further, I don't understand why we need to clear the value of an instance variable whose lifetime is that of the request.
There was a problem hiding this comment.
also, the reference to the user association will add an extra database dip (SELECT 'users'.* FROM 'users' WHERE 'users'.'id' = [user_id]) every time a UserLevel is created/updated.
(this query should be bypassed via Rails' automatic inverse_of inference, but doesn't seem to be working as expected..)
There was a problem hiding this comment.
There are tests that cause user_levels to get loaded and cached, then directly insert new user_levels and expect to see different results. This pointed out a theoretical problem: if in the course of a request, a user_level got created, and then a different caller asked for user_levels again before the end of the request, the new user_level would not be returned. Using the ActiveRecord callback to clear the cache on user_level insert or update is simple and robust.
There was a problem hiding this comment.
(I added a fix in #10745 that should make this code not add an extra query).
|
|
||
| %br/ | ||
| This level is in #{@level.script_levels.count} scripts: | ||
| This level is in #{@level.script_levels.to_a.count} scripts: |
There was a problem hiding this comment.
Again, I'll note that this is unrelated (for the benefit of future readers of this PR).
|
Can the PR have a more descriptive title? |
| if @user_level_cache_by_script[script_id].nil? | ||
| # make a database dip to get user_levels | ||
| @user_level_cache_by_script[script_id] = | ||
| user_levels.where(script_id: script_id).index_by(&:level_id) |
There was a problem hiding this comment.
This should be UserLevel.where...?
There was a problem hiding this comment.
This is existing code that was moved, will leave it as is for this PR
There was a problem hiding this comment.
Ignore my comment, this is definitely correct as is.
ashercodeorg
left a comment
There was a problem hiding this comment.
LGTM, other than comments and appropriate tests
|
|
||
| # returns if all user levels for this script have been cached | ||
| def user_levels_cached_for_script?(script_id) | ||
| @user_level_cache_by_script.try(:[], script_id).nil? ? false : true |
There was a problem hiding this comment.
This could be simplified to !@user_level_cache_by_script.try(:[], script_id).nil??
| user_levels. | ||
| where(script_id: script.id). | ||
| index_by(&:level_id) | ||
| cached_user_levels_by_script(script.id) |
There was a problem hiding this comment.
Though my belief is we do not, I do want to ask the question anyways. Do we want to gate this on Script.should_cache? or similar?
| if @user_level_cache_by_script[script_id].nil? | ||
| # make a database dip to get user_levels | ||
| @user_level_cache_by_script[script_id] = | ||
| user_levels.where(script_id: script_id).index_by(&:level_id) |
There was a problem hiding this comment.
Ignore my comment, this is definitely correct as is.
|
Please rename the PR title, it is sufficiently vague to be non-helpful. |
|
Should this be submitted or dropped or otherwise? |
This change removes a redundant user_level database dip on every level-rendering page load. user_level database reads are a significant source of where we spend our time. This reduces redundant user_level dips 3 -> 2. (Getting 2 down to 1 turns out to be a larger and non-trivial project which we can pursue separately.)