Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1319153004:
Cleanup ResourceLoadPriority setting (Closed)
|
Created:
5 years, 3 months ago by Nate Chapin Modified:
5 years, 3 months ago CC:
blink-reviews, gavinp+loader_chromium.org, kinuko+watch, tyoshino+watch_chromium.org, Yoav Weiss Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionCleanup ResourceLoadPriority setting
Split ResourceFetcher::loadPriority() into 3 functions:
* typeToPriority(), which converts Resource::Type into its default ResourceLoadPriority
* modifyPriorityForExperiments(), which contains all of the currently-running experimental logic.
* loadPriority() remains the entry point, and contains permanent special cases (e.g., synchronous loads).
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201667
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Move experiments to FrameFetchContext, add test #Patch Set 4 : More test cleanup #
Total comments: 5
Patch Set 5 : Remove secureURL #Patch Set 6 : Guess who uploaded without compiling locally #
Messages
Total messages: 30 (11 generated)
japhet@chromium.org changed reviewers: + mkwst@chromium.org, pmeenan@chromium.org
mkwst: PTAL pmeenan: I'd appreciate your opinion as to whether this is overall more readable/hackable, and a double-check that I correctly translated your experiments.
On 2015/08/31 18:37:25, Nate Chapin wrote: > mkwst: PTAL > > pmeenan: I'd appreciate your opinion as to whether this is overall more > readable/hackable, and a double-check that I correctly translated your > experiments. I'll map it out in the morning to make sure that the resulting priority levels match up with the originals. I'm a bit concerned about the readability (for the experiment path) though because of all of the relative shifts - makes it really hard to figure out what the actual resulting priority will be without walking through the logic. The relative adjustments to the enum could potentially blow past the bounds if the logic isn't exactly right.
On 2015/08/31 20:15:43, Pat Meenan wrote: > On 2015/08/31 18:37:25, Nate Chapin wrote: > > mkwst: PTAL > > > > pmeenan: I'd appreciate your opinion as to whether this is overall more > > readable/hackable, and a double-check that I correctly translated your > > experiments. > > I'll map it out in the morning to make sure that the resulting priority levels > match up with the originals. I'm a bit concerned about the readability (for the > experiment path) though because of all of the relative shifts - makes it really > hard to figure out what the actual resulting priority will be without walking > through the logic. The relative adjustments to the enum could potentially blow > past the bounds if the logic isn't exactly right. Agreed on the need for bounds-precaution. There's a reason I included the RELEASE_ASSERT for it :) I think this style is definitely more readable for the general increase priorities flag. It's a little less cut and dried for the script-specific cases.
On 2015/08/31 at 20:59:01, japhet wrote: > On 2015/08/31 20:15:43, Pat Meenan wrote: > > On 2015/08/31 18:37:25, Nate Chapin wrote: > > > mkwst: PTAL > > > > > > pmeenan: I'd appreciate your opinion as to whether this is overall more > > > readable/hackable, and a double-check that I correctly translated your > > > experiments. > > > > I'll map it out in the morning to make sure that the resulting priority levels > > match up with the originals. I'm a bit concerned about the readability (for the > > experiment path) though because of all of the relative shifts - makes it really > > hard to figure out what the actual resulting priority will be without walking > > through the logic. The relative adjustments to the enum could potentially blow > > past the bounds if the logic isn't exactly right. > > Agreed on the need for bounds-precaution. There's a reason I included the RELEASE_ASSERT for it :) > > I think this style is definitely more readable for the general increase priorities flag. It's a little less cut and dried for the script-specific cases. I agree that the change is a bit complicated, though I think it's better than the previous state. Can you add unit tests for this method (which might involve exposing typeToPriority for testing)? That would give us even more confidence that this patch isn't changing behavior. Cross-time-zone preemptive LGTM if you add some tests, and pmeenan's happy.
On 2015/09/01 13:41:37, Mike West (buried) wrote:
> On 2015/08/31 at 20:59:01, japhet wrote:
> > On 2015/08/31 20:15:43, Pat Meenan wrote:
> > > On 2015/08/31 18:37:25, Nate Chapin wrote:
> > > > mkwst: PTAL
> > > >
> > > > pmeenan: I'd appreciate your opinion as to whether this is overall more
> > > > readable/hackable, and a double-check that I correctly translated your
> > > > experiments.
> > >
> > > I'll map it out in the morning to make sure that the resulting priority
> levels
> > > match up with the originals. I'm a bit concerned about the readability
(for
> the
> > > experiment path) though because of all of the relative shifts - makes it
> really
> > > hard to figure out what the actual resulting priority will be without
> walking
> > > through the logic. The relative adjustments to the enum could potentially
> blow
> > > past the bounds if the logic isn't exactly right.
> >
> > Agreed on the need for bounds-precaution. There's a reason I included the
> RELEASE_ASSERT for it :)
> >
> > I think this style is definitely more readable for the general increase
> priorities flag. It's a little less cut and dried for the script-specific
cases.
>
> I agree that the change is a bit complicated, though I think it's better than
> the previous state. Can you add unit tests for this method (which might
involve
> exposing typeToPriority for testing)? That would give us even more confidence
> that this patch isn't changing behavior.
>
> Cross-time-zone preemptive LGTM if you add some tests, and pmeenan's happy.
not LGTM - some of the priorities end up being different than the original logic
How about taking just the script part of the logic with exlicit levels and using
that but the relative adjustments for everything else? The incremental
adjustment logic for scripts isn't keeping the same levels for some combinations
of late-body and async. For example, an early-body async script when "increase
priorities" is enabled but "increase async script priorities" is not:
Original code would exit on the first check with a Low priority:
if (FetchRequest::LazyLoad == request.defer())
return increaseAsyncScriptPriority ? ResourceLoadPriorityMedium :
ResourceLoadPriorityLow;
The new logic would set it to Medium:
- Initially set to Medium as the default script priority
- Increased to High because "increase priorities" is set and it is a script tag
- Lowered to Medium in the script-specific block because "increase async script
priorities" is not set
On 2015/09/01 14:11:58, Pat Meenan wrote: > On 2015/09/01 13:41:37, Mike West (buried) wrote: > > On 2015/08/31 at 20:59:01, japhet wrote: > > > On 2015/08/31 20:15:43, Pat Meenan wrote: > > > > On 2015/08/31 18:37:25, Nate Chapin wrote: > > > > > mkwst: PTAL > > > > > > > > > > pmeenan: I'd appreciate your opinion as to whether this is overall more > > > > > readable/hackable, and a double-check that I correctly translated your > > > > > experiments. > > > > > > > > I'll map it out in the morning to make sure that the resulting priority > > levels > > > > match up with the originals. I'm a bit concerned about the readability > (for > > the > > > > experiment path) though because of all of the relative shifts - makes it > > really > > > > hard to figure out what the actual resulting priority will be without > > walking > > > > through the logic. The relative adjustments to the enum could potentially > > blow > > > > past the bounds if the logic isn't exactly right. > > > > > > Agreed on the need for bounds-precaution. There's a reason I included the > > RELEASE_ASSERT for it :) > > > > > > I think this style is definitely more readable for the general increase > > priorities flag. It's a little less cut and dried for the script-specific > cases. > > > > I agree that the change is a bit complicated, though I think it's better than > > the previous state. Can you add unit tests for this method (which might > involve > > exposing typeToPriority for testing)? That would give us even more confidence > > that this patch isn't changing behavior. > > > > Cross-time-zone preemptive LGTM if you add some tests, and pmeenan's happy. > > not LGTM - some of the priorities end up being different than the original logic > > How about taking just the script part of the logic with exlicit levels and using > that but the relative adjustments for everything else? The incremental > adjustment logic for scripts isn't keeping the same levels for some combinations > of late-body and async. For example, an early-body async script when "increase > priorities" is enabled but "increase async script priorities" is not: > > Original code would exit on the first check with a Low priority: > > if (FetchRequest::LazyLoad == request.defer()) > return increaseAsyncScriptPriority ? ResourceLoadPriorityMedium : > ResourceLoadPriorityLow; > > The new logic would set it to Medium: > > - Initially set to Medium as the default script priority > - Increased to High because "increase priorities" is set and it is a script tag > - Lowered to Medium in the script-specific block because "increase async script > priorities" is not set We could also add more gates to the initial script increase and not do that for async scripts but I'm worried that we wouldn't catch all of the possible combinations.
Ok, updated. 1. Moved experiments to FrameFetchContext. Partly to reduce the number of functions on FetchContext, but mostly to better enable.... 2. Testing. Did some much-needed cleanup in FrameFetchContextTest, and added a test that covers ~every statement in modifyPriorityForExperiments(). Determined the expected values based on the current values on trunk. 3. Fixed the obvious (aforementioned) behavior change. If async scripts have their priority increased, ignore any effects of the generic increased priority flag.
The CQ bit was checked by japhet@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/1319153004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1319153004/40001
LGTM. I'm assuming the lifetime of the context is the same as the lifetime of the fetcher (to make sure the early/late state of scripts is consistent and gets reset with each navigation).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by japhet@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/1319153004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1319153004/60001
mkwst: would you mind giving this another look, especially for the cleanup FrameFetchContextTest.cpp https://codereview.chromium.org/1319153004/diff/60001/Source/core/loader/Fram... File Source/core/loader/FrameFetchContextTest.cpp (right): https://codereview.chromium.org/1319153004/diff/60001/Source/core/loader/Fram... Source/core/loader/FrameFetchContextTest.cpp:50: class StubFrameLoaderClientWithParent final : public EmptyFrameLoaderClient { Moved without modification from farther down in the file. https://codereview.chromium.org/1319153004/diff/60001/Source/core/loader/Fram... Source/core/loader/FrameFetchContextTest.cpp:74: class StubFrameOwner : public NoBaseWillBeGarbageCollectedFinalized<StubFrameOwner>, public FrameOwner { Moved without modification from farther down in the file.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
lgtm https://codereview.chromium.org/1319153004/diff/60001/Source/core/loader/Fram... File Source/core/loader/FrameFetchContextTest.cpp (right): https://codereview.chromium.org/1319153004/diff/60001/Source/core/loader/Fram... Source/core/loader/FrameFetchContextTest.cpp:125: RefPtrWillBePersistent<DocumentLoader> documentLoader; It might be nice to update these to use the right naming convention for members in a future patch. https://codereview.chromium.org/1319153004/diff/60001/Source/core/loader/Fram... Source/core/loader/FrameFetchContextTest.cpp:140: : secureURL(ParsedURLString, "https://secureorigin.test/image.png") Maybe just inline this directly into line 142? I don't see anyone else using "secureURL".
https://codereview.chromium.org/1319153004/diff/60001/Source/core/loader/Fram... File Source/core/loader/FrameFetchContextTest.cpp (right): https://codereview.chromium.org/1319153004/diff/60001/Source/core/loader/Fram... Source/core/loader/FrameFetchContextTest.cpp:140: : secureURL(ParsedURLString, "https://secureorigin.test/image.png") On 2015/09/02 17:17:12, dcheng wrote: > Maybe just inline this directly into line 142? I don't see anyone else using > "secureURL". Done.
The CQ bit was checked by japhet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, pmeenan@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1319153004/#ps80001 (title: "Remove secureURL")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1319153004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1319153004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by japhet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pmeenan@chromium.org, mkwst@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1319153004/#ps100001 (title: "Guess who uploaded without compiling locally")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1319153004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1319153004/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201667 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
