Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1532873003:
PlzNavigate: add initial traces for new TBM benchmarks. (Closed)
|
Created:
5 years ago by carlosk Modified:
4 years, 11 months ago CC:
benjhayden, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, kinuko, loading-reviews_chromium.org, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: add initial traces for new TBM benchmarks.
Adds new TRACE events in code to track some navigation related timings
in PlzNavigate. These will be collected by the new TBM benchmarks that
will be added in a following change.
Design doc describing the added traces: https://docs.google.com/a/chromium.org/document/d/1dyJejYZouBwyL6lCd5IJ3nU74lQKtCPra0M5tXrbIm4/edit?usp=sharing
Additions and changes not listed there:
- NavigationTiming navigationStart: I added this instant event with global scope as it seems to make sense to have it when tracking navigations, marking with a line over all processes the moment the navigation request is issues (coincides with the navigationStart time from the Navigation Timing API).
- RenderFrameHostImpl BeforeUnload: renamed from RenderFrameHostImpl::BeforeUnload as it looked like a method call trace when in fact that method doesn't exist. This is in fact an ASYNC trace that tracks in the browser the time taken to execute the beforeUnload call in the renderer and get its reply.
Note: I also wanted to "fix" the trace named RenderFrameHostManager:Navigate to add an extra ":" but I noticed in codesearch that there are references to that specific trace name in other parts of the code base.
BUG=529643
Committed: https://crrev.com/c13951a2c9f30a13d03a1a0310f9a79b66656c36
Cr-Commit-Position: refs/heads/master@{#367157}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Address CR comments #
Total comments: 5
Patch Set 3 : Address CR comments. #Patch Set 4 : Rebase. #
Total comments: 8
Patch Set 5 : Address CR comments. #Patch Set 6 : Fixed crash issue for synchronous navigations. #
Messages
Total messages: 31 (12 generated)
carlosk@chromium.org changed reviewers: + clamy@chromium.org
clamy@ PTAL.
Description was changed from ========== PlzNavigate: add initial traces for new TBM benchmarks. Adds new TRACE events in code to track some navigation related timings in PlzNavigate. These will be collected by the new TBM benchmarks that will be added in a followin change. BUG=529643 ========== to ========== PlzNavigate: add initial traces for new TBM benchmarks. Adds new TRACE events in code to track some navigation related timings in PlzNavigate. These will be collected by the new TBM benchmarks that will be added in a followin change. BUG=529643 ==========
Thanks! A few questions in the comments, as well as a more general one: I see that a certain number of traces are for main frame only, why can't we have them for subframes as well? In particular, I don't see a good reason why we should not have the timeToCommit for subframes as well. https://codereview.chromium.org/1532873003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1532873003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:285: TRACE_EVENT_ASYNC_END_WITH_TIMESTAMP0( I'm not sure about the naming of the measurement - start may be a bit vague. For exemple, do we register the trace when the net stack receives the request or when the various throttles have executed and the network request actually starts? https://codereview.chromium.org/1532873003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1532873003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:303: TRACE_EVENT_ASYNC_BEGIN_WITH_TIMESTAMP1( Maybe add a comment about what the handle and the ftn id are used for? https://codereview.chromium.org/1532873003/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1532873003/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.cc:1139: TRACE_EVENT_ASYNC_END1("navigation", "RenderFrameHostImpl BeforeUnload", this, Is it an issue if this is called without the begin first? There are paths in BeforeUnload where the start won't be called. https://codereview.chromium.org/1532873003/diff/1/content/browser/loader/navi... File content/browser/loader/navigation_url_loader_impl.cc (right): https://codereview.chromium.org/1532873003/diff/1/content/browser/loader/navi... content/browser/loader/navigation_url_loader_impl.cc:29: TRACE_EVENT_ASYNC_BEGIN_WITH_TIMESTAMP1( This is called on the UI thread while the end happens on the IO thread. Is that an issue? https://codereview.chromium.org/1532873003/diff/1/content/browser/loader/navi... File content/browser/loader/navigation_url_loader_impl_core.cc (right): https://codereview.chromium.org/1532873003/diff/1/content/browser/loader/navi... content/browser/loader/navigation_url_loader_impl_core.cc:87: TRACE_EVENT_ASYNC_END0("navigation", "Navigation redirectDelay", this); nit: I would leave the DCHECK_CURRENTLY_ON first. Same below. https://codereview.chromium.org/1532873003/diff/1/content/browser/loader/reso... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1532873003/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1122: // PlzNavigate: this log happens from |NavigationRequest::OnRequestStarted| nit: no ||? I think we prefer to use those for arguments, and not function names.
Thanks! On 2015/12/18 15:42:11, clamy wrote: > Thanks! A few questions in the comments, as well as a more general one: I see > that a certain number of traces are for main frame only, why can't we have them > for subframes as well? In particular, I don't see a good reason why we should > not have the timeToCommit for subframes as well. It is my intention to make them all -- timeToIOStart, timeToResponseStarted, timeToCommit -- all work for subframes. I quickly tried doing so but only succeeded for timeToResponseStarted and I thought it would be confusing to only have one of them working. So as subframe tracking is not necessary for the first version of the TBM tests I decided to leave it for later but make it clear that these are only valid for the main frame for now. https://codereview.chromium.org/1532873003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1532873003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:285: TRACE_EVENT_ASYNC_END_WITH_TIMESTAMP0( On 2015/12/18 15:42:11, clamy wrote: > I'm not sure about the naming of the measurement - start may be a bit vague. For > exemple, do we register the trace when the net stack receives the request or > when the various throttles have executed and the network request actually > starts? Looking at the code this is triggered from, it happens along with the call to ResourceDispatcherHostImpl::BeginNavigationRequest which checks with ResourceHandlers (that go through the throttles as well). So it's before the net stack actually starts. It seems this is OK as a point of measurement but I agree we might need to rename it in this case. I'm thinking "Navigation timeToIOStartBeforeThrottles" but I'm all ears for better ideas. Another option is to move this further down so that this is tracked after the handlers/throttlers. Or even adding another measurement at that point. https://codereview.chromium.org/1532873003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1532873003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:303: TRACE_EVENT_ASYNC_BEGIN_WITH_TIMESTAMP1( On 2015/12/18 15:42:11, clamy wrote: > Maybe add a comment about what the handle and the ftn id are used for? Done. https://codereview.chromium.org/1532873003/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1532873003/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.cc:1139: TRACE_EVENT_ASYNC_END1("navigation", "RenderFrameHostImpl BeforeUnload", this, On 2015/12/18 15:42:11, clamy wrote: > Is it an issue if this is called without the begin first? There are paths in > BeforeUnload where the start won't be called. From a tracing perspective no. I have to rely on this mechanin --no trace if no recorded BEGIN -- in other places as well (like for redirects). No if there are paths where there was a beforeUnload handled by the renderer and I didn't get it right, then it is a problem with my choice of trace points. https://codereview.chromium.org/1532873003/diff/1/content/browser/loader/navi... File content/browser/loader/navigation_url_loader_impl.cc (right): https://codereview.chromium.org/1532873003/diff/1/content/browser/loader/navi... content/browser/loader/navigation_url_loader_impl.cc:29: TRACE_EVENT_ASYNC_BEGIN_WITH_TIMESTAMP1( On 2015/12/18 15:42:11, clamy wrote: > This is called on the UI thread while the end happens on the IO thread. Is that > an issue? Nope, it's supported behavior: https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/c... https://codereview.chromium.org/1532873003/diff/1/content/browser/loader/navi... File content/browser/loader/navigation_url_loader_impl_core.cc (right): https://codereview.chromium.org/1532873003/diff/1/content/browser/loader/navi... content/browser/loader/navigation_url_loader_impl_core.cc:87: TRACE_EVENT_ASYNC_END0("navigation", "Navigation redirectDelay", this); On 2015/12/18 15:42:11, clamy wrote: > nit: I would leave the DCHECK_CURRENTLY_ON first. Same below. Done. https://codereview.chromium.org/1532873003/diff/1/content/browser/loader/reso... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1532873003/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1122: // PlzNavigate: this log happens from |NavigationRequest::OnRequestStarted| On 2015/12/18 15:42:11, clamy wrote: > nit: no ||? I think we prefer to use those for arguments, and not function > names. Done.
Thanks! A few more comments (mostly on comments). https://codereview.chromium.org/1532873003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1532873003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:285: TRACE_EVENT_ASYNC_END_WITH_TIMESTAMP0( On 2015/12/18 17:23:21, carlosk wrote: > On 2015/12/18 15:42:11, clamy wrote: > > I'm not sure about the naming of the measurement - start may be a bit vague. > For > > exemple, do we register the trace when the net stack receives the request or > > when the various throttles have executed and the network request actually > > starts? > > Looking at the code this is triggered from, it happens along with the call to > ResourceDispatcherHostImpl::BeginNavigationRequest which checks with > ResourceHandlers (that go through the throttles as well). So it's before the net > stack actually starts. > > It seems this is OK as a point of measurement but I agree we might need to > rename it in this case. I'm thinking "Navigation timeToIOStartBeforeThrottles" > but I'm all ears for better ideas. > > Another option is to move this further down so that this is tracked after the > handlers/throttlers. Or even adding another measurement at that point. I'm fine with the measurement happening when it happens. For the name, maybe timeToNetworkStack (I'm not very fond of it either - hopefully Nasko will have better ideas). https://codereview.chromium.org/1532873003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1532873003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:303: TRACE_EVENT_ASYNC_BEGIN_WITH_TIMESTAMP1( On 2015/12/18 17:23:21, carlosk wrote: > On 2015/12/18 15:42:11, clamy wrote: > > Maybe add a comment about what the handle and the ftn id are used for? > > Done. I'd rather have the comment all packed together than interspread between lines. I find it hard to read this way. If you don't want to do a single comment at the top, you can do: parameter, /* short comment */ but not have the comments in between lines. https://codereview.chromium.org/1532873003/diff/20001/content/browser/loader/... File content/browser/loader/navigation_url_loader_impl.cc (right): https://codereview.chromium.org/1532873003/diff/20001/content/browser/loader/... content/browser/loader/navigation_url_loader_impl.cc:30: // category and name Same thing about the comments. https://codereview.chromium.org/1532873003/diff/20001/content/browser/loader/... File content/browser/loader/navigation_url_loader_impl_core.cc (right): https://codereview.chromium.org/1532873003/diff/20001/content/browser/loader/... content/browser/loader/navigation_url_loader_impl_core.cc:85: "&NavigationURLLoaderImplCore", this, Why the & here? Also as mentioned somewhere else, comments.
Thanks. https://codereview.chromium.org/1532873003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1532873003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:285: TRACE_EVENT_ASYNC_END_WITH_TIMESTAMP0( On 2015/12/21 10:43:32, clamy wrote: > On 2015/12/18 17:23:21, carlosk wrote: > > On 2015/12/18 15:42:11, clamy wrote: > > > I'm not sure about the naming of the measurement - start may be a bit vague. > > For > > > exemple, do we register the trace when the net stack receives the request or > > > when the various throttles have executed and the network request actually > > > starts? > > > > Looking at the code this is triggered from, it happens along with the call to > > ResourceDispatcherHostImpl::BeginNavigationRequest which checks with > > ResourceHandlers (that go through the throttles as well). So it's before the > net > > stack actually starts. > > > > It seems this is OK as a point of measurement but I agree we might need to > > rename it in this case. I'm thinking "Navigation timeToIOStartBeforeThrottles" > > but I'm all ears for better ideas. > > > > Another option is to move this further down so that this is tracked after the > > handlers/throttlers. Or even adding another measurement at that point. > > I'm fine with the measurement happening when it happens. For the name, maybe > timeToNetworkStack (I'm not very fond of it either - hopefully Nasko will have > better ideas). Done. https://codereview.chromium.org/1532873003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1532873003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:303: TRACE_EVENT_ASYNC_BEGIN_WITH_TIMESTAMP1( On 2015/12/21 10:43:32, clamy wrote: > On 2015/12/18 17:23:21, carlosk wrote: > > On 2015/12/18 15:42:11, clamy wrote: > > > Maybe add a comment about what the handle and the ftn id are used for? > > > > Done. > > I'd rather have the comment all packed together than interspread between lines. > I find it hard to read this way. > If you don't want to do a single comment at the top, you can do: > parameter, /* short comment */ but not have the comments in between lines. Done. https://codereview.chromium.org/1532873003/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1532873003/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:380: TRACE_EVENT1("navigation", "RenderFrameHostManager::Navigate", I reverted this change because I found references to the current (incorrect IMO) trace name in other places. https://codereview.chromium.org/1532873003/diff/20001/content/browser/loader/... File content/browser/loader/navigation_url_loader_impl.cc (right): https://codereview.chromium.org/1532873003/diff/20001/content/browser/loader/... content/browser/loader/navigation_url_loader_impl.cc:30: // category and name On 2015/12/21 10:43:32, clamy wrote: > Same thing about the comments. Done. https://codereview.chromium.org/1532873003/diff/20001/content/browser/loader/... File content/browser/loader/navigation_url_loader_impl_core.cc (right): https://codereview.chromium.org/1532873003/diff/20001/content/browser/loader/... content/browser/loader/navigation_url_loader_impl_core.cc:85: "&NavigationURLLoaderImplCore", this, On 2015/12/21 10:43:32, clamy wrote: > Why the & here? > Also as mentioned somewhere else, comments. Done.
Thanks! Lgtm with a few nits. https://codereview.chromium.org/1532873003/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1532873003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:288: } nit: skip one line after }. https://codereview.chromium.org/1532873003/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1532873003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:295: if (frame_tree_node->IsMainFrame()) { nit: add a TODO to extend these to subframes as well. https://codereview.chromium.org/1532873003/diff/60001/content/browser/loader/... File content/browser/loader/navigation_url_loader_impl.cc (right): https://codereview.chromium.org/1532873003/diff/60001/content/browser/loader/... content/browser/loader/navigation_url_loader_impl.cc:29: // For the trace below we're using the NavigationURLLoaderImplCore as the nit: skip one line above comment. https://codereview.chromium.org/1532873003/diff/60001/content/browser/loader/... File content/browser/loader/navigation_url_loader_impl_core.cc (right): https://codereview.chromium.org/1532873003/diff/60001/content/browser/loader/... content/browser/loader/navigation_url_loader_impl_core.cc:79: // For the trace below we're using the NavigationURLLoaderImplCore as the nit: skip one line above comment.
carlosk@chromium.org changed reviewers: + nasko@chromium.org
Thanks clamy@. nasko@ PTAL. https://codereview.chromium.org/1532873003/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1532873003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:288: } On 2015/12/22 10:55:23, clamy wrote: > nit: skip one line after }. Done. https://codereview.chromium.org/1532873003/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1532873003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:295: if (frame_tree_node->IsMainFrame()) { On 2015/12/22 10:55:24, clamy wrote: > nit: add a TODO to extend these to subframes as well. Done. In other places as well. https://codereview.chromium.org/1532873003/diff/60001/content/browser/loader/... File content/browser/loader/navigation_url_loader_impl.cc (right): https://codereview.chromium.org/1532873003/diff/60001/content/browser/loader/... content/browser/loader/navigation_url_loader_impl.cc:29: // For the trace below we're using the NavigationURLLoaderImplCore as the On 2015/12/22 10:55:24, clamy wrote: > nit: skip one line above comment. Done. https://codereview.chromium.org/1532873003/diff/60001/content/browser/loader/... File content/browser/loader/navigation_url_loader_impl_core.cc (right): https://codereview.chromium.org/1532873003/diff/60001/content/browser/loader/... content/browser/loader/navigation_url_loader_impl_core.cc:79: // For the trace below we're using the NavigationURLLoaderImplCore as the On 2015/12/22 10:55:24, clamy wrote: > nit: skip one line above comment. Done.
nduca@, benjhayden@: FYI.
Where's the design doc that explains how these relate and fit together? Nit wise, they're all named oddly --- we don't typically name things with spaces... not sure what is right here, but its something I would appreciate a conversation about. I notice they're all in the navigation category. Why not benchmark, too? The this is done will require that whoever wants these metrics records with a navigation category turned on... thats non normative.
Even "WebcontentsImpl Loading" ideally would get named sanely. Another good question: what happens when we start tracing *after* loading has begun? We'd be missing some of these traces. Ben has been experimenting with an idea for solving this where you can say to tracing "make sure this async event gets created even in corner cases like when tracing starts midway through." Though this wouldn't be an issue for telemetry versions of the benchmark, if you wanted loading info to work for any trace then you'd need to solve that problem.
Description was changed from ========== PlzNavigate: add initial traces for new TBM benchmarks. Adds new TRACE events in code to track some navigation related timings in PlzNavigate. These will be collected by the new TBM benchmarks that will be added in a followin change. BUG=529643 ========== to ========== PlzNavigate: add initial traces for new TBM benchmarks. Adds new TRACE events in code to track some navigation related timings in PlzNavigate. These will be collected by the new TBM benchmarks that will be added in a following change. Design doc describing the added traces: https://docs.google.com/a/chromium.org/document/d/1dyJejYZouBwyL6lCd5IJ3nU74l... Additions and changes not listed there: - NavigationTiming navigationStart: I added this instant event with global scope as it seems to make sense to have it when tracking navigations, marking with a line over all processes the moment the navigation request is issues (coincides with the navigationStart time from the Navigation Timing API). - RenderFrameHostImpl BeforeUnload: renamed from RenderFrameHostImpl::BeforeUnload as it looked like a method call trace when in fact that method doesn't exist. This is in fact an ASYNC trace that tracks in the browser the time taken to execute the beforeUnload call in the renderer and get its reply. BUG=529643 ==========
Description was changed from ========== PlzNavigate: add initial traces for new TBM benchmarks. Adds new TRACE events in code to track some navigation related timings in PlzNavigate. These will be collected by the new TBM benchmarks that will be added in a following change. Design doc describing the added traces: https://docs.google.com/a/chromium.org/document/d/1dyJejYZouBwyL6lCd5IJ3nU74l... Additions and changes not listed there: - NavigationTiming navigationStart: I added this instant event with global scope as it seems to make sense to have it when tracking navigations, marking with a line over all processes the moment the navigation request is issues (coincides with the navigationStart time from the Navigation Timing API). - RenderFrameHostImpl BeforeUnload: renamed from RenderFrameHostImpl::BeforeUnload as it looked like a method call trace when in fact that method doesn't exist. This is in fact an ASYNC trace that tracks in the browser the time taken to execute the beforeUnload call in the renderer and get its reply. BUG=529643 ========== to ========== PlzNavigate: add initial traces for new TBM benchmarks. Adds new TRACE events in code to track some navigation related timings in PlzNavigate. These will be collected by the new TBM benchmarks that will be added in a following change. Design doc describing the added traces: https://docs.google.com/a/chromium.org/document/d/1dyJejYZouBwyL6lCd5IJ3nU74l... Additions and changes not listed there: - NavigationTiming navigationStart: I added this instant event with global scope as it seems to make sense to have it when tracking navigations, marking with a line over all processes the moment the navigation request is issues (coincides with the navigationStart time from the Navigation Timing API). - RenderFrameHostImpl BeforeUnload: renamed from RenderFrameHostImpl::BeforeUnload as it looked like a method call trace when in fact that method doesn't exist. This is in fact an ASYNC trace that tracks in the browser the time taken to execute the beforeUnload call in the renderer and get its reply. Note: I also wanted to "fix" the trace named RenderFrameHostManager:Navigate to add an extra ":" but I noticed in codesearch that there are references to that specific trace name in other parts of the code base. BUG=529643 ==========
On 2015/12/22 19:27:25, nduca wrote: > Where's the design doc that explains how these relate and fit together? My bad, I forgot to reference it from the description. Fixed that and added comments on the traces I touched that were not described there. > Nit wise, they're all named oddly --- we don't typically name things with > spaces... not sure what is right here, but its something I would appreciate a > conversation about. I did look at the tracing documentation and found no information on naming conventions or category usage so I just followed what seemed to make sense given what's in code right now. My logic for using spaces -- and even switching one case from "::" to a space -- was to tell apart traces scoped to a method call of traces unrelated to a specific method call. It makes total sense to label a scoped trace of a method with the method name. But for the other case using "::" is confusing as at first look I'd assume it refers to an existing method. As you noticed the already existent "WebcontentsImpl Loading" already use spaces and seems to match the non-method-scoped case I described. > I notice they're all in the navigation category. Why not benchmark, too? The > this is done will require that whoever wants these metrics records with a > navigation category turned on... thats non normative. I chose the "navigation" category because it already existed and these are metrics are related to navigation. I can add "benchmark" to them too but it wasn't obvious to me I should. I didn't understand your last statements though (non-questions). On 2015/12/22 19:31:04, nduca wrote: > Even "WebcontentsImpl Loading" ideally would get named sanely. Already commented above. > Another good question: what happens when we start tracing *after* loading has > begun? We'd be missing some of these traces. Ben has been experimenting with an > idea for solving this where you can say to tracing "make sure this async event > gets created even in corner cases like when tracing starts midway through." > Though this wouldn't be an issue for telemetry versions of the benchmark, if you > wanted loading info to work for any trace then you'd need to solve that problem. I'm trying here to fulfill the tracing requirements for the page_cycler-like TBM benchmark we're trying to create. As that is not an issue here I wasn't even aware it was a problem.
Patchset #6 (id:100001) has been deleted
carlosk@chromium.org changed reviewers: + avi@chromium.org
avi@: PTAL.
lgtm Trace ALL the things! °O°/
Thanks. I'm going to commit this as is and address nduca@'s comments in the follow up CL with the TBM benchmark itself (crrev.com/1317883003).
The CQ bit was checked by carlosk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org Link to the patchset: https://codereview.chromium.org/1532873003/#ps120001 (title: "Fixed crash issue for synchronous navigations.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1532873003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1532873003/120001
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: add initial traces for new TBM benchmarks. Adds new TRACE events in code to track some navigation related timings in PlzNavigate. These will be collected by the new TBM benchmarks that will be added in a following change. Design doc describing the added traces: https://docs.google.com/a/chromium.org/document/d/1dyJejYZouBwyL6lCd5IJ3nU74l... Additions and changes not listed there: - NavigationTiming navigationStart: I added this instant event with global scope as it seems to make sense to have it when tracking navigations, marking with a line over all processes the moment the navigation request is issues (coincides with the navigationStart time from the Navigation Timing API). - RenderFrameHostImpl BeforeUnload: renamed from RenderFrameHostImpl::BeforeUnload as it looked like a method call trace when in fact that method doesn't exist. This is in fact an ASYNC trace that tracks in the browser the time taken to execute the beforeUnload call in the renderer and get its reply. Note: I also wanted to "fix" the trace named RenderFrameHostManager:Navigate to add an extra ":" but I noticed in codesearch that there are references to that specific trace name in other parts of the code base. BUG=529643 ========== to ========== PlzNavigate: add initial traces for new TBM benchmarks. Adds new TRACE events in code to track some navigation related timings in PlzNavigate. These will be collected by the new TBM benchmarks that will be added in a following change. Design doc describing the added traces: https://docs.google.com/a/chromium.org/document/d/1dyJejYZouBwyL6lCd5IJ3nU74l... Additions and changes not listed there: - NavigationTiming navigationStart: I added this instant event with global scope as it seems to make sense to have it when tracking navigations, marking with a line over all processes the moment the navigation request is issues (coincides with the navigationStart time from the Navigation Timing API). - RenderFrameHostImpl BeforeUnload: renamed from RenderFrameHostImpl::BeforeUnload as it looked like a method call trace when in fact that method doesn't exist. This is in fact an ASYNC trace that tracks in the browser the time taken to execute the beforeUnload call in the renderer and get its reply. Note: I also wanted to "fix" the trace named RenderFrameHostManager:Navigate to add an extra ":" but I noticed in codesearch that there are references to that specific trace name in other parts of the code base. BUG=529643 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: add initial traces for new TBM benchmarks. Adds new TRACE events in code to track some navigation related timings in PlzNavigate. These will be collected by the new TBM benchmarks that will be added in a following change. Design doc describing the added traces: https://docs.google.com/a/chromium.org/document/d/1dyJejYZouBwyL6lCd5IJ3nU74l... Additions and changes not listed there: - NavigationTiming navigationStart: I added this instant event with global scope as it seems to make sense to have it when tracking navigations, marking with a line over all processes the moment the navigation request is issues (coincides with the navigationStart time from the Navigation Timing API). - RenderFrameHostImpl BeforeUnload: renamed from RenderFrameHostImpl::BeforeUnload as it looked like a method call trace when in fact that method doesn't exist. This is in fact an ASYNC trace that tracks in the browser the time taken to execute the beforeUnload call in the renderer and get its reply. Note: I also wanted to "fix" the trace named RenderFrameHostManager:Navigate to add an extra ":" but I noticed in codesearch that there are references to that specific trace name in other parts of the code base. BUG=529643 ========== to ========== PlzNavigate: add initial traces for new TBM benchmarks. Adds new TRACE events in code to track some navigation related timings in PlzNavigate. These will be collected by the new TBM benchmarks that will be added in a following change. Design doc describing the added traces: https://docs.google.com/a/chromium.org/document/d/1dyJejYZouBwyL6lCd5IJ3nU74l... Additions and changes not listed there: - NavigationTiming navigationStart: I added this instant event with global scope as it seems to make sense to have it when tracking navigations, marking with a line over all processes the moment the navigation request is issues (coincides with the navigationStart time from the Navigation Timing API). - RenderFrameHostImpl BeforeUnload: renamed from RenderFrameHostImpl::BeforeUnload as it looked like a method call trace when in fact that method doesn't exist. This is in fact an ASYNC trace that tracks in the browser the time taken to execute the beforeUnload call in the renderer and get its reply. Note: I also wanted to "fix" the trace named RenderFrameHostManager:Navigate to add an extra ":" but I noticed in codesearch that there are references to that specific trace name in other parts of the code base. BUG=529643 Committed: https://crrev.com/c13951a2c9f30a13d03a1a0310f9a79b66656c36 Cr-Commit-Position: refs/heads/master@{#367157} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c13951a2c9f30a13d03a1a0310f9a79b66656c36 Cr-Commit-Position: refs/heads/master@{#367157}
Message was sent while issue was closed.
nduca@chromium.org changed reviewers: + nduca@chromium.org
Message was sent while issue was closed.
pretty bummed that you landed this with my comments outstanding. :/
Message was sent while issue was closed.
On 2016/01/11 19:47:14, nduca wrote: > pretty bummed that you landed this with my comments outstanding. :/ Did you see my comment #21 (or the email I sent you) stating I'd address them in the follow up CL? Last week I also copied your concerns and my replies there so that we can resume addressing them. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
