Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2066763002:
[WeakMemoryCache] Remove Resource from MemoryCache just before destructed (Closed)
|
Created:
4 years, 6 months ago by hiroshige Modified:
4 years, 6 months ago CC:
chromium-reviews, blink-reviews, loading-reviews+fetch_chromium.org, tyoshino+watch_chromium.org, Nate Chapin, gavinp+loader_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[WeakMemoryCache] Remove Resource from MemoryCache just before destructed
Previously, if Weak MemoryCache is enabled, Resource could be silently
destructed and MemoryCache's live/dead sizes were not updated.
This may cause larger |m_live/deadSize| and more aggressive pruning.
This CL calls MemoryCache::remove() just before a Resource is destructed
to make the MemoryCache sizes correct.
This CL should be reverted before going to beta.
BUG=603462
Committed: https://crrev.com/4706b08cb45742e3b0dcc73ba443522d0cce28da
Cr-Commit-Position: refs/heads/master@{#401536}
Patch Set 1 #Patch Set 2 #Patch Set 3 #
Total comments: 3
Messages
Total messages: 31 (13 generated)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066763002/1
Description was changed from ========== [WeakMemoryCache] Remove Resource from MemoryCache just before destructed Previously, Resource could be silently destructed if Weak MemoryCache is enabled and MemoryCache's live/dead sizes were not updated (and thus were larger than the correct values). This CL calls MemoryCache::remove() just before a Resource is destructed to make the MemoryCache sizes correct. This CL should be reverted before going to beta. BUG=603462 ========== to ========== [WeakMemoryCache] Remove Resource from MemoryCache just before destructed Previously, if Weak MemoryCache is enabled, Resource could be silently destructed and MemoryCache's live/dead sizes were not updated. This may cause larger |m_live/deadSize| and more aggressive pruning. This CL calls MemoryCache::remove() just before a Resource is destructed to make the MemoryCache sizes correct. This CL should be reverted before going to beta. BUG=603462 ==========
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066763002/20001
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066763002/40001
hiroshige@chromium.org changed reviewers: + japhet@chromium.org, yhirano@chromium.org
PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yhirano@chromium.org changed reviewers: + oilpan-reviews@chromium.org
I think this CL should be reviewed by oilpan reviewers.
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2066763002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/MemoryCache.cpp (right): https://codereview.chromium.org/2066763002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/MemoryCache.cpp:89: void MemoryCacheEntry::clearResourceWeak(Visitor* visitor) This is not called when the MemoryCacheEntry is dead (i.e., clearResourceWeak is called only when the MemoryCacheEntry was traced (i.e., alive)). Is it okay?
https://codereview.chromium.org/2066763002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/MemoryCache.cpp (right): https://codereview.chromium.org/2066763002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/MemoryCache.cpp:89: void MemoryCacheEntry::clearResourceWeak(Visitor* visitor) On 2016/06/15 08:21:05, haraken wrote: > > This is not called when the MemoryCacheEntry is dead (i.e., clearResourceWeak is > called only when the MemoryCacheEntry was traced (i.e., alive)). Is it okay? It's OK (because MemoryCacheEntry is alive unless it is evicted from MemoryCache).
LGTM
lgtm
japhet@, could you take a look as a core/fetch OWNER? Thanks.
LGTM bordering on rubber-stamp... https://codereview.chromium.org/2066763002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/MemoryCache.cpp (right): https://codereview.chromium.org/2066763002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/MemoryCache.cpp:82: visitor->template registerWeakMembers<MemoryCacheEntry, &MemoryCacheEntry::clearResourceWeak>(this); I don't speak templates, and it stuns me that this is valid syntax. :D
The CQ bit was checked by hiroshige@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066763002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by hiroshige@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066763002/40001
Message was sent while issue was closed.
Description was changed from ========== [WeakMemoryCache] Remove Resource from MemoryCache just before destructed Previously, if Weak MemoryCache is enabled, Resource could be silently destructed and MemoryCache's live/dead sizes were not updated. This may cause larger |m_live/deadSize| and more aggressive pruning. This CL calls MemoryCache::remove() just before a Resource is destructed to make the MemoryCache sizes correct. This CL should be reverted before going to beta. BUG=603462 ========== to ========== [WeakMemoryCache] Remove Resource from MemoryCache just before destructed Previously, if Weak MemoryCache is enabled, Resource could be silently destructed and MemoryCache's live/dead sizes were not updated. This may cause larger |m_live/deadSize| and more aggressive pruning. This CL calls MemoryCache::remove() just before a Resource is destructed to make the MemoryCache sizes correct. This CL should be reverted before going to beta. BUG=603462 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [WeakMemoryCache] Remove Resource from MemoryCache just before destructed Previously, if Weak MemoryCache is enabled, Resource could be silently destructed and MemoryCache's live/dead sizes were not updated. This may cause larger |m_live/deadSize| and more aggressive pruning. This CL calls MemoryCache::remove() just before a Resource is destructed to make the MemoryCache sizes correct. This CL should be reverted before going to beta. BUG=603462 ========== to ========== [WeakMemoryCache] Remove Resource from MemoryCache just before destructed Previously, if Weak MemoryCache is enabled, Resource could be silently destructed and MemoryCache's live/dead sizes were not updated. This may cause larger |m_live/deadSize| and more aggressive pruning. This CL calls MemoryCache::remove() just before a Resource is destructed to make the MemoryCache sizes correct. This CL should be reverted before going to beta. BUG=603462 Committed: https://crrev.com/4706b08cb45742e3b0dcc73ba443522d0cce28da Cr-Commit-Position: refs/heads/master@{#401536} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4706b08cb45742e3b0dcc73ba443522d0cce28da Cr-Commit-Position: refs/heads/master@{#401536}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2110683005/ by hiroshige@chromium.org. The reason for reverting is: Reverting field-trial CLs before going to beta, as planned.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
