Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2444783002:
Add trace event for complete network request (Closed)
|
Created:
4 years, 2 months ago by dproy Modified:
4 years, 1 month ago CC:
blink-reviews, chromium-reviews, gavinp+loader_chromium.org, Nate Chapin, loading-reviews+fetch_chromium.org, Primiano Tucci (use gerrit), tyoshino+watch_chromium.org, Yoav Weiss Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd trace event for complete network request
Network instrumentation in Chrome is being revamped. This is a first in
a series of CLs that introduces new trace points for all the lifecycle
phases of a network request.
The current CL introduces an async slice representing the time from the
beginning of a network request in Blink (more precisely, from the time
when Blink generates a unique id for the request) to when all of the
response has been received by Blink or Blink has been notified that the
request failed. It does not include the time to notify the
ResourceClients, which means, among other things, it does not include
the time to parse the response.
BUG=
Committed: https://crrev.com/7624dfa8d32243a7f631975c825ef1c9db6b67fb
Cr-Commit-Position: refs/heads/master@{#431747}
Patch Set 1 #
Total comments: 19
Patch Set 2 : Address review comments #
Total comments: 8
Patch Set 3 : Address comments by chiniforooshan@ #Patch Set 4 : Remove empty beginResourceLoad function #
Total comments: 10
Patch Set 5 : Nuke old tracepoints. Add priority instant events. Add TracedValue. #
Total comments: 20
Patch Set 6 : Address comments by caseq@ #Patch Set 7 : duplicate upload #Patch Set 8 : Calm down compiler with return value #
Messages
Total messages: 43 (15 generated)
dproy@chromium.org changed reviewers: + caseq@chromium.org, chiniforooshan@chromium.org
PTAL. Is this a good starting point for the network instrumentation work?
Thanks a lot for doing this! https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:501: TRACE_EVENT1("blink", "ResourceFetcher::requestResource", "url", Please create a bug and add a TODO here, referencing the bug, to eventually delete these events now that we are introducing network_instrumentation. Please add the TODO in other similar locations in this file, too (i.e. for blink.net events). https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:509: identifier, request.url().getString().utf8().data()); Do you think we can move this (and the line before in which an id is created) to the beginning of the function, right after the UMA update? https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/tracing/NetworkInstrumentation.cpp (right): https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/tracing/NetworkInstrumentation.cpp:15: TRACE_EVENT_ASYNC_BEGIN1(kNetInstrumentationCategory, kResourceLoadTitle, I think it is recommended that new codes use nestable async events. https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/tracing/NetworkInstrumentation.cpp:16: resourceID, "url", TRACE_STR_COPY(url)); TRACE_ID_WITH_SCOPE(kBlinkResourceID, TRACE_ID_LOCAL(resourceID)) https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/tracing/NetworkInstrumentation.h (right): https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/tracing/NetworkInstrumentation.h:12: const char kNetInstrumentationCategory[] = "NetworkInstrumentation"; Lower case category names seem to be more popular. How about something like "net.v2"? https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/tracing/NetworkInstrumentation.h:12: const char kNetInstrumentationCategory[] = "NetworkInstrumentation"; What do you think about TRACE_DISABLED_BY_DEFAULT(...) for now, to not pollute TV while we are in the work-in-progress phase, and maybe enable it by default later when net instrumentation is in a good shape?
https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:509: identifier, request.url().getString().utf8().data()); On 2016/10/24 19:25:36, chiniforooshan wrote: > Do you think we can move this (and the line before in which an id is created) to > the beginning of the function, right after the UMA update? Do we really need it in the same place? I think it's important that it's _after_ the bail out -- otherwise we would have to take care of properly terminating the event. I'd actually consider moving it down to actual start load, considering the amount of bail outs here. Also, let's pass blink strings as they are to the instrumentation call -- or maybe just pass the entire request? https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1176: network_instrumentation::endResourceLoad(resource->identifier()); let's also pass success / fail flag down to the instrumentation method. https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/tracing/NetworkInstrumentation.cpp (right): https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/tracing/NetworkInstrumentation.cpp:15: TRACE_EVENT_ASYNC_BEGIN1(kNetInstrumentationCategory, kResourceLoadTitle, On 2016/10/24 19:25:37, chiniforooshan wrote: > I think it is recommended that new codes use nestable async events. +1 https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/tracing/NetworkInstrumentation.h (right): https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/tracing/NetworkInstrumentation.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. This rather needs to be somewhere under platorm/network -- tracing isn't supposed to "know" anything about networking.
yoav@yoav.ws changed reviewers: + yoav@yoav.ws
https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:509: identifier, request.url().getString().utf8().data()); On 2016/10/24 22:47:46, caseq wrote: > On 2016/10/24 19:25:36, chiniforooshan wrote: > > Do you think we can move this (and the line before in which an id is created) > to > > the beginning of the function, right after the UMA update? > > Do we really need it in the same place? I think it's important that it's _after_ > the bail out -- otherwise we would have to take care of properly terminating the > event. I'd actually consider moving it down to actual start load, considering > the amount of bail outs here. I think it's important to be able to instrument this particular function, as it's known to be fairly expensive. Starting the instrumentation at the start load would ignore that. > > Also, let's pass blink strings as they are to the instrumentation call -- or > maybe just pass the entire request?
https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:509: identifier, request.url().getString().utf8().data()); On 2016/10/24 22:47:46, caseq wrote: > On 2016/10/24 19:25:36, chiniforooshan wrote: > > Do you think we can move this (and the line before in which an id is created) > to > > the beginning of the function, right after the UMA update? > > Do we really need it in the same place? I think it's important that it's _after_ > the bail out -- otherwise we would have to take care of properly terminating the > event. I'd actually consider moving it down to actual start load, considering > the amount of bail outs here. > > Also, let's pass blink strings as they are to the instrumentation call -- or > maybe just pass the entire request? Good point about terminating the event! Can we deal with that by making BeginResourceLoad a scoped object that closes the event in its destructor if we don't tell it otherwise, i.e. when a resource request is actually going to be sent? IMO it's better to cover the logic in this function; people may be specially interested in memcache stuff?
The CQ bit was checked by chiniforooshan@chromium.org to run a CQ dry run
The CQ bit was unchecked by chiniforooshan@chromium.org
Updated to address all the comments. PTAL :) https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:501: TRACE_EVENT1("blink", "ResourceFetcher::requestResource", "url", On 2016/10/24 19:25:36, chiniforooshan wrote: > Please create a bug and add a TODO here, referencing the bug, to eventually > delete these events now that we are introducing network_instrumentation. Please > add the TODO in other similar locations in this file, too (i.e. for http://blink.net > events). Done. https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:509: identifier, request.url().getString().utf8().data()); On 2016/10/25 18:00:40, chiniforooshan wrote: > On 2016/10/24 22:47:46, caseq wrote: > > On 2016/10/24 19:25:36, chiniforooshan wrote: > > > Do you think we can move this (and the line before in which an id is > created) > > to > > > the beginning of the function, right after the UMA update? > > > > Do we really need it in the same place? I think it's important that it's > _after_ > > the bail out -- otherwise we would have to take care of properly terminating > the > > event. I'd actually consider moving it down to actual start load, considering > > the amount of bail outs here. > > > > Also, let's pass blink strings as they are to the instrumentation call -- or > > maybe just pass the entire request? > > Good point about terminating the event! Can we deal with that by making > BeginResourceLoad a scoped object that closes the event in its destructor if we > don't tell it otherwise, i.e. when a resource request is actually going to be > sent? IMO it's better to cover the logic in this function; people may be > specially interested in memcache stuff? I added a scoped tracker. Does the overhead look reasonable? https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1176: network_instrumentation::endResourceLoad(resource->identifier()); On 2016/10/24 22:47:46, caseq wrote: > let's also pass success / fail flag down to the instrumentation method. Done. https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/tracing/NetworkInstrumentation.cpp (right): https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/tracing/NetworkInstrumentation.cpp:15: TRACE_EVENT_ASYNC_BEGIN1(kNetInstrumentationCategory, kResourceLoadTitle, On 2016/10/24 22:47:46, caseq wrote: > On 2016/10/24 19:25:37, chiniforooshan wrote: > > I think it is recommended that new codes use nestable async events. > > +1 Done. https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/tracing/NetworkInstrumentation.cpp:16: resourceID, "url", TRACE_STR_COPY(url)); On 2016/10/24 19:25:37, chiniforooshan wrote: > TRACE_ID_WITH_SCOPE(kBlinkResourceID, TRACE_ID_LOCAL(resourceID)) Done. https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/tracing/NetworkInstrumentation.h (right): https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/tracing/NetworkInstrumentation.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/10/24 22:47:46, caseq wrote: > This rather needs to be somewhere under platorm/network -- tracing isn't > supposed to "know" anything about networking. Done. https://codereview.chromium.org/2444783002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/tracing/NetworkInstrumentation.h:12: const char kNetInstrumentationCategory[] = "NetworkInstrumentation"; On 2016/10/24 19:25:37, chiniforooshan wrote: > Lower case category names seem to be more popular. How about something like > "net.v2"? Changed the cat name. Disabled by default sounds good.
lgtm with only minor comments. https://codereview.chromium.org/2444783002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2444783002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:640: scopedResourceLoadTracker.doNotCloseSliceAtEndOfScope(); Move this right after "if (!startLoad(resource)) return nullptr"? Saves one line :) https://codereview.chromium.org/2444783002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/NetworkInstrumentation.cpp (right): https://codereview.chromium.org/2444783002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkInstrumentation.cpp:45: void beginResourceLoad(unsigned long resourceID, Is it necessary to have this function? I would move the following to the ScopedResourceLoadTracer constructor. https://codereview.chromium.org/2444783002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkInstrumentation.cpp:51: TRACE_STR_COPY(urlString.utf8().data())); is TRACE_STR_COPY(request.url().getString().utf8().data()) too long? If not, write it all in one line and remove the include CString from the beginning. https://codereview.chromium.org/2444783002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/NetworkInstrumentation.h (right): https://codereview.chromium.org/2444783002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkInstrumentation.h:25: const blink::ResourceRequest request; I don't see why we need to store the request?
Addressed comments. +caseq I'll add owner reviewers after you do another pass on this. https://codereview.chromium.org/2444783002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2444783002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:640: scopedResourceLoadTracker.doNotCloseSliceAtEndOfScope(); On 2016/11/03 15:50:36, chiniforooshan wrote: > Move this right after "if (!startLoad(resource)) return nullptr"? Saves one line > :) Done. https://codereview.chromium.org/2444783002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/NetworkInstrumentation.cpp (right): https://codereview.chromium.org/2444783002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkInstrumentation.cpp:45: void beginResourceLoad(unsigned long resourceID, On 2016/11/03 15:50:36, chiniforooshan wrote: > Is it necessary to have this function? I would move the following to the > ScopedResourceLoadTracer constructor. I moved it inside. https://codereview.chromium.org/2444783002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkInstrumentation.cpp:51: TRACE_STR_COPY(urlString.utf8().data())); On 2016/11/03 15:50:36, chiniforooshan wrote: > is TRACE_STR_COPY(request.url().getString().utf8().data()) too long? If not, > write it all in one line and remove the include CString from the beginning. Done. https://codereview.chromium.org/2444783002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/NetworkInstrumentation.h (right): https://codereview.chromium.org/2444783002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkInstrumentation.h:25: const blink::ResourceRequest request; On 2016/11/03 15:50:36, chiniforooshan wrote: > I don't see why we need to store the request? You're right - there's no reason to store it. Fixed.
fmeawad@chromium.org changed reviewers: + fmeawad@chromium.org
https://codereview.chromium.org/2444783002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/NetworkInstrumentation.cpp (right): https://codereview.chromium.org/2444783002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkInstrumentation.cpp:15: const char kNetInstrumentationCategory[] = TRACE_DISABLED_BY_DEFAULT("net.v2"); Can you use a category name that does not include a .v2 or something like that to minimize migration issues, use a new good name, for example "network"
Looks mostly good. Let's nuke the old instrumentation now and let's add an instant event for priority change. Also, let's use TracedValue for the start argument and add the initial priority value there -- priorities are quite useful. https://codereview.chromium.org/2444783002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2444783002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1055: TRACE_EVENT_ASYNC_STEP_INTO0("blink.net", "Resource", resource->identifier(), Let's just throw away these, they don't event seem to be properly started. https://codereview.chromium.org/2444783002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1130: TRACE_EVENT_ASYNC_END0("blink.net", "Resource", resource->identifier()); ditto. https://codereview.chromium.org/2444783002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1188: TRACE_EVENT_ASYNC_END0("blink.net", "Resource", resource->identifier()); ditto. https://codereview.chromium.org/2444783002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1437: TRACE_EVENT_ASYNC_STEP_INTO1("blink.net", "Resource", Let's add the new instrumentation support for that right now.
Ready for review. PTAL. https://codereview.chromium.org/2444783002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2444783002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1055: TRACE_EVENT_ASYNC_STEP_INTO0("blink.net", "Resource", resource->identifier(), On 2016/11/03 22:43:47, caseq wrote: > Let's just throw away these, they don't event seem to be properly started. Done. https://codereview.chromium.org/2444783002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1130: TRACE_EVENT_ASYNC_END0("blink.net", "Resource", resource->identifier()); On 2016/11/03 22:43:47, caseq wrote: > ditto. Done. https://codereview.chromium.org/2444783002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1188: TRACE_EVENT_ASYNC_END0("blink.net", "Resource", resource->identifier()); On 2016/11/03 22:43:47, caseq wrote: > ditto. Done. https://codereview.chromium.org/2444783002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1437: TRACE_EVENT_ASYNC_STEP_INTO1("blink.net", "Resource", On 2016/11/03 22:43:47, caseq wrote: > Let's add the new instrumentation support for that right now. Done. https://codereview.chromium.org/2444783002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/NetworkInstrumentation.cpp (right): https://codereview.chromium.org/2444783002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkInstrumentation.cpp:15: const char kNetInstrumentationCategory[] = TRACE_DISABLED_BY_DEFAULT("net.v2"); On 2016/11/03 22:18:58, fmeawad wrote: > Can you use a category name that does not include a .v2 or something like that > to minimize migration issues, use a new good name, for example "network" Done.
csharrison@chromium.org changed reviewers: + csharrison@chromium.org
https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:504: // TODO(dproy): Remove this. http://crbug.com/659666 drive-by: This trace is important for tracking CPU time of requesting a single resource. Please make sure that the new probe points capture this before deleting this trace, as I am actively working on the CPU overhead it covers.
https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:504: // TODO(dproy): Remove this. http://crbug.com/659666 On 2016/11/08 15:20:18, Charlie Harrison wrote: > drive-by: This trace is important for tracking CPU time of requesting a single > resource. Please make sure that the new probe points capture this before > deleting this trace, as I am actively working on the CPU overhead it covers. Do you mean just this single trace in particular? IIUC it seems like this is only tracking the requestResource function (as opposed to say, the cost of the rest of the request lifecycle, including other parts in the renderer and all the parts in the browser.) If this particular function is very important to instrument, we can add probes to capture that.
https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:504: // TODO(dproy): Remove this. http://crbug.com/659666 On 2016/11/08 16:31:50, dproy wrote: > On 2016/11/08 15:20:18, Charlie Harrison wrote: > > drive-by: This trace is important for tracking CPU time of requesting a single > > resource. Please make sure that the new probe points capture this before > > deleting this trace, as I am actively working on the CPU overhead it covers. > > Do you mean just this single trace in particular? IIUC it seems like this is > only tracking the requestResource function (as opposed to say, the cost of the > rest of the request lifecycle, including other parts in the renderer and all the > parts in the browser.) > > If this particular function is very important to instrument, we can add probes > to capture that. Yep this single method does a lot of work, including going out to the FrameFetchContext, and //content in multiple places. It is the main driver in forming the request and sending the resource request IPC to the browser process. While you're here, do you mind moving it to the top of the method, underneath the SCOPED_BLINK_UMA_HISTOGRAM?
https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:504: // TODO(dproy): Remove this. http://crbug.com/659666 On 2016/11/08 16:34:26, Charlie Harrison wrote: > On 2016/11/08 16:31:50, dproy wrote: > > On 2016/11/08 15:20:18, Charlie Harrison wrote: > > > drive-by: This trace is important for tracking CPU time of requesting a > single > > > resource. Please make sure that the new probe points capture this before > > > deleting this trace, as I am actively working on the CPU overhead it covers. > > > > Do you mean just this single trace in particular? IIUC it seems like this is > > only tracking the requestResource function (as opposed to say, the cost of the > > rest of the request lifecycle, including other parts in the renderer and all > the > > parts in the browser.) > > > > If this particular function is very important to instrument, we can add probes > > to capture that. > > Yep this single method does a lot of work, including going out to the > FrameFetchContext, and //content in multiple places. It is the main driver in > forming the request and sending the resource request IPC to the browser process. > > While you're here, do you mind moving it to the top of the method, underneath > the SCOPED_BLINK_UMA_HISTOGRAM? Ah I see. This CL is about tracking the entire network request - let's keep it that way. Once this lands, I'll send a quick follow up CL that removes this trace point but adds a subslice tracking this function (and I'll put the start point of that subslice at the beginning of this function underneath the UMA, as you wanted.)
https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:504: // TODO(dproy): Remove this. http://crbug.com/659666 On 2016/11/08 16:45:35, dproy wrote: > On 2016/11/08 16:34:26, Charlie Harrison wrote: > > On 2016/11/08 16:31:50, dproy wrote: > > > On 2016/11/08 15:20:18, Charlie Harrison wrote: > > > > drive-by: This trace is important for tracking CPU time of requesting a > > single > > > > resource. Please make sure that the new probe points capture this before > > > > deleting this trace, as I am actively working on the CPU overhead it > covers. > > > > > > Do you mean just this single trace in particular? IIUC it seems like this is > > > only tracking the requestResource function (as opposed to say, the cost of > the > > > rest of the request lifecycle, including other parts in the renderer and all > > the > > > parts in the browser.) > > > > > > If this particular function is very important to instrument, we can add > probes > > > to capture that. > > > > Yep this single method does a lot of work, including going out to the > > FrameFetchContext, and //content in multiple places. It is the main driver in > > forming the request and sending the resource request IPC to the browser > process. > > > > While you're here, do you mind moving it to the top of the method, underneath > > the SCOPED_BLINK_UMA_HISTOGRAM? > > Ah I see. This CL is about tracking the entire network request - let's keep it > that way. Once this lands, I'll send a quick follow up CL that removes this > trace point but adds a subslice tracking this function (and I'll put the start > point of that subslice at the beginning of this function underneath the UMA, as > you wanted.) That plan sounds great to me. Just to clarify, will a "subslice" have similar semantics to this probe point in the UI? I often e.g. double click this event to find aggregate metrics for it in a single trace. Would that still work when it is refactored as a subslice?
On 2016/11/08 17:34:04, Charlie Harrison wrote: > https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:504: // TODO(dproy): > Remove this. http://crbug.com/659666 > On 2016/11/08 16:45:35, dproy wrote: > > On 2016/11/08 16:34:26, Charlie Harrison wrote: > > > On 2016/11/08 16:31:50, dproy wrote: > > > > On 2016/11/08 15:20:18, Charlie Harrison wrote: > > > > > drive-by: This trace is important for tracking CPU time of requesting a > > > single > > > > > resource. Please make sure that the new probe points capture this before > > > > > deleting this trace, as I am actively working on the CPU overhead it > > covers. > > > > > > > > Do you mean just this single trace in particular? IIUC it seems like this > is > > > > only tracking the requestResource function (as opposed to say, the cost of > > the > > > > rest of the request lifecycle, including other parts in the renderer and > all > > > the > > > > parts in the browser.) > > > > > > > > If this particular function is very important to instrument, we can add > > probes > > > > to capture that. > > > > > > Yep this single method does a lot of work, including going out to the > > > FrameFetchContext, and //content in multiple places. It is the main driver > in > > > forming the request and sending the resource request IPC to the browser > > process. > > > > > > While you're here, do you mind moving it to the top of the method, > underneath > > > the SCOPED_BLINK_UMA_HISTOGRAM? > > > > Ah I see. This CL is about tracking the entire network request - let's keep it > > that way. Once this lands, I'll send a quick follow up CL that removes this > > trace point but adds a subslice tracking this function (and I'll put the start > > point of that subslice at the beginning of this function underneath the UMA, > as > > you wanted.) > > That plan sounds great to me. Just to clarify, will a "subslice" have similar > semantics to this probe point in the UI? I often e.g. double click this event to > find aggregate metrics for it in a single trace. Would that still work when it > is refactored as a subslice? I didn't even know double clicking events was a thing until now :) I just checked and it should work the same.
lgtm % bunch of comments. https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:515: network_instrumentation::initialResourcePrioritySet( let's unify this with resourcePriorityChanged, I'd rather handle one event instead of two on the processing side, and I'm sure we would be able to tell one from another if we really need by nesting -- so just name it something like resourcePrioritySet and use the same on both call sites. https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/NetworkInstrumentation.cpp (right): https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkInstrumentation.cpp:46: if (this->closeSliceAtEndOfScope) we generally don't mention this-> explicitly. https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkInstrumentation.cpp:47: endResourceLoad(this->resourceID, RequestOutcome::Fail); ditto. https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkInstrumentation.cpp:51: this->closeSliceAtEndOfScope = false; ditto. https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkInstrumentation.cpp:57: data->setInteger("initialPriority", priority); ditto. https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/NetworkInstrumentation.h (right): https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkInstrumentation.h:10: #include "platform/network/ResourceRequest.h" please forward declare here instead. https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkInstrumentation.h:24: bool closeSliceAtEndOfScope; use m_ prefix. Also, how about a more specific name, e.g. m_resourceLoadingCancelled? https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkInstrumentation.h:25: const unsigned long resourceID; m_resourceId https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkInstrumentation.h:38: const char* RequestOutcomeToString(RequestOutcome); do we need to expose this one?
https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:515: network_instrumentation::initialResourcePrioritySet( On 2016/11/08 18:24:32, caseq wrote: > let's unify this with resourcePriorityChanged, I'd rather handle one event > instead of two on the processing side, and I'm sure we would be able to tell one > from another if we really need by nesting -- so just name it something like > resourcePrioritySet and use the same on both call sites. Done. https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/NetworkInstrumentation.cpp (right): https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkInstrumentation.cpp:46: if (this->closeSliceAtEndOfScope) On 2016/11/08 18:24:32, caseq wrote: > we generally don't mention this-> explicitly. Done for all the `this` cases. https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/NetworkInstrumentation.h (right): https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkInstrumentation.h:10: #include "platform/network/ResourceRequest.h" On 2016/11/08 18:24:32, caseq wrote: > please forward declare here instead. Done. https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkInstrumentation.h:24: bool closeSliceAtEndOfScope; On 2016/11/08 18:24:32, caseq wrote: > use m_ prefix. Also, how about a more specific name, e.g. > m_resourceLoadingCancelled? Done. I changed it to m_resourceLoadContinuesBeyondScope, because otherwise m_resourceLoadingCancelled = true by default and that was confusing. https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkInstrumentation.h:25: const unsigned long resourceID; On 2016/11/08 18:24:32, caseq wrote: > m_resourceId Done. https://codereview.chromium.org/2444783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkInstrumentation.h:38: const char* RequestOutcomeToString(RequestOutcome); On 2016/11/08 18:24:32, caseq wrote: > do we need to expose this one? Nope - thanks for pointing this out.
dproy@chromium.org changed reviewers: + mkwst@chromium.org
mkwst@ - Adding you for owner review.
LGTM.
The CQ bit was checked by dproy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chiniforooshan@chromium.org, caseq@chromium.org Link to the patchset: https://codereview.chromium.org/2444783002/#ps120001 (title: "duplicate upload")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by dproy@chromium.org
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
Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by dproy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org, chiniforooshan@chromium.org, mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/2444783002/#ps140001 (title: "Calm down compiler with return value")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add trace event for complete network request Network instrumentation in Chrome is being revamped. This is a first in a series of CLs that introduces new trace points for all the lifecycle phases of a network request. The current CL introduces an async slice representing the time from the beginning of a network request in Blink (more precisely, from the time when Blink generates a unique id for the request) to when all of the response has been received by Blink or Blink has been notified that the request failed. It does not include the time to notify the ResourceClients, which means, among other things, it does not include the time to parse the response. BUG= ========== to ========== Add trace event for complete network request Network instrumentation in Chrome is being revamped. This is a first in a series of CLs that introduces new trace points for all the lifecycle phases of a network request. The current CL introduces an async slice representing the time from the beginning of a network request in Blink (more precisely, from the time when Blink generates a unique id for the request) to when all of the response has been received by Blink or Blink has been notified that the request failed. It does not include the time to notify the ResourceClients, which means, among other things, it does not include the time to parse the response. BUG= Committed: https://crrev.com/7624dfa8d32243a7f631975c825ef1c9db6b67fb Cr-Commit-Position: refs/heads/master@{#431747} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/7624dfa8d32243a7f631975c825ef1c9db6b67fb Cr-Commit-Position: refs/heads/master@{#431747} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
