|
|
Created:
4 years ago by Charlie Harrison Modified:
4 years ago CC:
chromium-reviews, blink-reviews, loading-reviews+fetch_chromium.org, tyoshino+watch_chromium.org, Nate Chapin, gavinp+loader_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't remove CSSPreloaderResourceClient for unused speculative markup preloads
The CSS @import scanner attaches a passive resource client to a css
preload request. This passive client should not affect the policy
decisions of the preload and should just observe notifications passively.
This patch fixes a bug where removing a passive client from an otherwise
unused preload ends up cancelling it, which removes the preload from
memory cache. This is very wrong behavior, and causes the optimization
to be less effective, and report bad metrics.
Simply not removing the client will not cause the resource to live
longer than necessary, because the client holds only weak references
to the resource.
BUG=670295, 662999
Committed: https://crrev.com/dff0384e2ab5ade040a08a38e72eece575ea0aee
Cr-Commit-Position: refs/heads/master@{#436312}
Patch Set 1 #Patch Set 2 : allow dynamic link preload removal #
Total comments: 4
Patch Set 3 : yhirano review #
Messages
Total messages: 35 (20 generated)
csharrison@chromium.org changed reviewers: + yhirano@chromium.org, yoav@yoav.ws
PTAL!
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Hm hold off, a couple layout tests failures look related, sorry.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Bots are happy after I special cased link preloads. PTAL?
Thank you for working on this issue. https://codereview.chromium.org/2542183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2542183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:762: if (isUnusedPreload() && !isLinkPreload()) Is it possible to register a ResourceClient instead? I would like to keep this function simple as much as possible.
https://codereview.chromium.org/2542183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2542183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:762: if (isUnusedPreload() && !isLinkPreload()) On 2016/12/02 01:32:25, yhirano wrote: > Is it possible to register a ResourceClient instead? I would like to keep this > function simple as much as possible. Hm... so the CSSPreloaderResourceClient removes itself as soon as it gets the data it needs, which triggers this callback. We could have that client *not* remove itself, but that would entail other problematic things like link preloads that have "as=style" not cancelling if they are dynamically removed from the page.
Description was changed from ========== Do not cancel unused preloads if a passive client is removed. The CSS @import scanner attaches a passive resource client to a css preload request. This passive client should not affect the policy decisions of the preload and should just observe notifications passively. This patch fixes a bug where removing a passive client from an otherwise unused preload ends up cancelling it, which removes the preload from memory cache. This is very wrong behavior, and causes the optimization to be less effective, and report bad metrics. BUG=670295,662999 ========== to ========== Do not cancel unused preloads if a passive client is removed. The CSS @import scanner attaches a passive resource client to a css preload request. This passive client should not affect the policy decisions of the preload and should just observe notifications passively. This patch fixes a bug where removing a passive client from an otherwise unused preload ends up cancelling it, which removes the preload from memory cache. This is very wrong behavior, and causes the optimization to be less effective, and report bad metrics. BUG=670295,662999 ==========
yhirano@chromium.org changed reviewers: + hiroshige@chromium.org
+hiroshige@ fyi https://codereview.chromium.org/2542183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2542183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:762: if (isUnusedPreload() && !isLinkPreload()) On 2016/12/02 04:20:22, Charlie Harrison wrote: > On 2016/12/02 01:32:25, yhirano wrote: > > Is it possible to register a ResourceClient instead? I would like to keep this > > function simple as much as possible. > > Hm... so the CSSPreloaderResourceClient removes itself as soon as it gets the > data it needs, which triggers this callback. We could have that client *not* > remove itself, but that would entail other problematic things like link preloads > that have "as=style" not cancelling if they are dynamically removed from the > page. Then is it possible to stop removing itself only when isLinkPreload() returns false?
https://codereview.chromium.org/2542183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2542183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:762: if (isUnusedPreload() && !isLinkPreload()) On 2016/12/02 08:50:29, yhirano wrote: > On 2016/12/02 04:20:22, Charlie Harrison wrote: > > On 2016/12/02 01:32:25, yhirano wrote: > > > Is it possible to register a ResourceClient instead? I would like to keep > this > > > function simple as much as possible. > > > > Hm... so the CSSPreloaderResourceClient removes itself as soon as it gets the > > data it needs, which triggers this callback. We could have that client *not* > > remove itself, but that would entail other problematic things like link > preloads > > that have "as=style" not cancelling if they are dynamically removed from the > > page. > > > Then is it possible to stop removing itself only when isLinkPreload() returns > false? I think it might be possible now that CSSPreloaderResourceClient holds weak references to the resource. Let me think it through to make sure there are no gotchas. I'm not totally convinced that that solution is simpler than this one, because its action is more "at a distance", but I don't feel too strongly and I agree this method being simple is better for everyone.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Alright, the change you requested is running through the bots now. I *think* it should be safe because we don't support dynamic removal of link rel stylesheets causing cancellation of the request. I think probably in the future we should have ResourceFetcher register a passive ResourceClient for preloads, and have clearPreloads remove the client, causing request cancellation at clearPreloads time.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/02 15:58:13, Charlie Harrison wrote: > Alright, the change you requested is running through the bots now. I *think* it > should be safe because we don't support dynamic removal of link rel stylesheets > causing cancellation of the request. > > I think probably in the future we should have ResourceFetcher register a passive > ResourceClient for preloads, and have clearPreloads remove the client, causing > request cancellation at clearPreloads time. Thanks, lgtm.
Thanks! Yoav, I'll wait for your stamp to land, as this is related to the investigation you've been helping me with re: preload miss metrics. Let me also re-write the description and title.
Description was changed from ========== Do not cancel unused preloads if a passive client is removed. The CSS @import scanner attaches a passive resource client to a css preload request. This passive client should not affect the policy decisions of the preload and should just observe notifications passively. This patch fixes a bug where removing a passive client from an otherwise unused preload ends up cancelling it, which removes the preload from memory cache. This is very wrong behavior, and causes the optimization to be less effective, and report bad metrics. BUG=670295,662999 ========== to ========== Don't remove CSSPreloaderResourceClient for unused speculative markup preloads The CSS @import scanner attaches a passive resource client to a css preload request. This passive client should not affect the policy decisions of the preload and should just observe notifications passively. This patch fixes a bug where removing a passive client from an otherwise unused preload ends up cancelling it, which removes the preload from memory cache. This is very wrong behavior, and causes the optimization to be less effective, and report bad metrics. Simply not removing the client will not cause the resource to live longer than necessary, because the client holds only weak references to the resource. BUG=670295,662999 ==========
On 2016/12/05 04:03:26, Charlie Harrison wrote: > Thanks! Yoav, I'll wait for your stamp to land, as this is related to the > investigation you've been helping me with re: preload miss metrics. > > Let me also re-write the description and title. LGTM! I'm very happy with how this patch turned out. Thanks Charlie and yhirano :)
Thanks! Let me push to CQ.
The CQ bit was checked by csharrison@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480948886513290, "parent_rev": "6f43e87c220da73e4739d708a89100a9c7daad25", "commit_rev": "192f1c8922cc370258f554c879c9696803444fc2"}
Message was sent while issue was closed.
Description was changed from ========== Don't remove CSSPreloaderResourceClient for unused speculative markup preloads The CSS @import scanner attaches a passive resource client to a css preload request. This passive client should not affect the policy decisions of the preload and should just observe notifications passively. This patch fixes a bug where removing a passive client from an otherwise unused preload ends up cancelling it, which removes the preload from memory cache. This is very wrong behavior, and causes the optimization to be less effective, and report bad metrics. Simply not removing the client will not cause the resource to live longer than necessary, because the client holds only weak references to the resource. BUG=670295,662999 ========== to ========== Don't remove CSSPreloaderResourceClient for unused speculative markup preloads The CSS @import scanner attaches a passive resource client to a css preload request. This passive client should not affect the policy decisions of the preload and should just observe notifications passively. This patch fixes a bug where removing a passive client from an otherwise unused preload ends up cancelling it, which removes the preload from memory cache. This is very wrong behavior, and causes the optimization to be less effective, and report bad metrics. Simply not removing the client will not cause the resource to live longer than necessary, because the client holds only weak references to the resource. BUG=670295,662999 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Don't remove CSSPreloaderResourceClient for unused speculative markup preloads The CSS @import scanner attaches a passive resource client to a css preload request. This passive client should not affect the policy decisions of the preload and should just observe notifications passively. This patch fixes a bug where removing a passive client from an otherwise unused preload ends up cancelling it, which removes the preload from memory cache. This is very wrong behavior, and causes the optimization to be less effective, and report bad metrics. Simply not removing the client will not cause the resource to live longer than necessary, because the client holds only weak references to the resource. BUG=670295,662999 ========== to ========== Don't remove CSSPreloaderResourceClient for unused speculative markup preloads The CSS @import scanner attaches a passive resource client to a css preload request. This passive client should not affect the policy decisions of the preload and should just observe notifications passively. This patch fixes a bug where removing a passive client from an otherwise unused preload ends up cancelling it, which removes the preload from memory cache. This is very wrong behavior, and causes the optimization to be less effective, and report bad metrics. Simply not removing the client will not cause the resource to live longer than necessary, because the client holds only weak references to the resource. BUG=670295,662999 Committed: https://crrev.com/dff0384e2ab5ade040a08a38e72eece575ea0aee Cr-Commit-Position: refs/heads/master@{#436312} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/dff0384e2ab5ade040a08a38e72eece575ea0aee Cr-Commit-Position: refs/heads/master@{#436312} |