Fix memory leak in action view #27296
Conversation
|
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @kaspth (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review. Please see the contribution instructions for more information. |
printercu
force-pushed
the
improving_av
branch
from
ffc4f0c
to
ddd5b6c
Compare
|
No problem. I'll rebase when it's merged. Just don't want to resolve conflicts in the future. Do you plan to merge #27271? |
printercu
force-pushed
the
improving_av
branch
from
7553df9
to
3db2925
Compare
printercu
changed the title
|
Seems like everything is ready. |
|
|
||
| @details_keys = Concurrent::Map.new | ||
| # Make hash uniq. | ||
| alias_method :hash, :object_id |
There was a problem hiding this comment.
I don't understand this.
The whole point of this is to provide a key we can use to look up the value in the cache. If it has a new [hash] value each time, the cache is useless. 😕
There was a problem hiding this comment.
There are some details #27273 (comment)
It had been using hash before, but @tenderlove says it causes collisions.
There was a problem hiding this comment.
I don't know if everything will be fine with jRuby and other rubies.
Maybe I should change this to uuid?
UPD. Uh. I hope object_id means that it's uniq.
There was a problem hiding this comment.
My point is that it's not supposed to be unique. It's a cache key.
(I also see no such claim from @tenderlove.. where was that?)
There was a problem hiding this comment.
An object's hash value is not unique, so it's possible for this cache
key to produce colliding keys with no resolution. This commit move
cache on to the details key object itself, so we know that the digests
are always unique per details key object.
There was a problem hiding this comment.
I'm not really sure, how it can give collisions, while it also uses name, finder.rendered_format, dependencies in digests cache.
And name, prefix, partial, locals in templates cache.
However if we can have it uniq with single line, maybe it's good idea.
| def get(*) | ||
| ActionView::Digestor::PerExecutionCacheExpiry.before(nil) | ||
| super | ||
| end |
There was a problem hiding this comment.
@jeremy Can you please review it. It's about the feature of using template digestest in etag (#16527).
I had some tests failing (test_etag_reflects_implicit_template_digest and similar) with Expected Ok, got Not Modified and fixed them with this lines. Am I right, that it should work only in development, when digest cache is cleared on each request?
|
Rebased branch is ready. I'll push it after discussion, to not miss open comments. |
|
There are too many unrelated things changing in these commits, including rather cosmetic refactorings that later get reversed(?) Can you please rebase it down to just the changes you believe are necessary to address the leak? |
printercu
force-pushed
the
improving_av
branch
from
3db2925
to
ed9e7c1
Compare
|
Rebased. |
printercu
force-pushed
the
improving_av
branch
2 times, most recently
from
0988d78
to
3f24d5a
Compare
|
Reverted back |
printercu
force-pushed
the
improving_av
branch
3 times, most recently
from
101f030
to
bc2a624
Compare
|
ActionCable is failing again, everything else is fine. |
|
@matthewd anything prevents merging this one? |
|
There are still a mixture of changes that are either unrelated or unexplained... and I remain entirely unconvinced that the |
|
Can you please talk to @tenderlove about his notice on |
|
Created sample app to compare patched version with 5.0.1: https://github.com/printercu/rails_dev_leak_example |
|
Are you still interested in getting this reviewed? Is this still an issue on master? |
There is no need to clear DetailsKey cache, so it should eliminate memory leak.
|
I hope this is still useful. I've just rebased on master and run this tests https://github.com/rails/rails/pull/27296/files#diff-c3e4d6a7a9f83e67a19f2c90f3738443R71 - 2 of them are failed: not modified templates are recompiled on every request and memory is leaking. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
#27273 for details.
Here I moved digest cache back to Digestor. In this way, it's possible not to clear DetailsKey cache, and keys for cached templates will be permanent.
I need to add testcase for not leaking memory.I'm fixing action pack tests.