Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1922823003:
Add frame ID to LayoutAnalyzer trace event (Closed)
|
Created:
4 years, 8 months ago by Kunihiko Sakamoto Modified:
4 years, 7 months ago CC:
blink-reviews, chromium-reviews, kouhei (in TOK), loading-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd frame ID to LayoutAnalyzer trace event
This makes it easy to associate a LayoutAnalyzer event with other trace
events.
BUG=507790
Committed: https://crrev.com/6f19ad4e07796d9b62894e0b856b90ed0bd7649d
Cr-Commit-Position: refs/heads/master@{#390306}
Patch Set 1 #Patch Set 2 : Put frame ID to TracedValue #
Total comments: 4
Patch Set 3 : PRIxPTR #Messages
Total messages: 22 (7 generated)
Description was changed from ========== Add frame ID to LayoutAnalyzer trace event BUG= ========== to ========== Add frame ID to LayoutAnalyzer trace event This makes it easy to associate a LayoutAnalyzer event with other trace events. BUG=507790 ==========
ksakamoto@chromium.org changed reviewers: + benjhayden@chromium.org
Ben, PTAL? This is going to be a part of trace-based First Meaningful Paint.
Which other trace events do you need to associate with FrameView::performLayout? Did you want to look into designing a more efficient way to record this information? Are there any other chromium changes that will be required for FMP? Why not put the frame in the TracedValue returned by analyzerCounters()? It already contains a hostname (which might become redundant if the frame could be associated with the Frame object snapshots).
+Sami, is it possible to correlate the FrameBlameContext with the LocalFrame
pointer? It looks like there's a public/private layer boundary between them at
the WebLocalFrame{,Impl}.
On 2016/04/26 at 17:31:53, benjhayden wrote: > Are there any other chromium changes that will be required for FMP? Here's the full diff of blink changes: https://codereview.chromium.org/1773633003/ The frame association is pretty straightforward with this frameID: https://gist.github.com/irori/e5af3d2994f34b7a2a68ac596e5c9ceb#file-ttfmp-rb-... > Why not put the frame in the TracedValue returned by analyzerCounters()? That seems OK to me, but clearly I defer to Sakamoto-san.
Thanks Paul for adding context. On 2016/04/26 17:31:53, benjhayden_chromium wrote: > Which other trace events do you need to associate with FrameView::performLayout? devtools.timeline.Paint event and blink.user_timing events logged by DocumentLoadTiming / PaintTiming. > Did you want to look into designing a more efficient way to record this > information? Is LayoutAnalyzer's overhead too big to be used in benchmarks? I thought this is OK for trace-based implementation. I'm not going to enable this tracing by default. When we make this an UMA metric, certainly we need a more efficient approach. > Are there any other chromium changes that will be required for FMP? https://codereview.chromium.org/1773633003/ is the overall picture, the webfont part needs some refinements before landing though. I wanted to land this one first, so that Kouhei/Paul can start prototyping FMP metric for TBMv2 / lighthouse asap. > Why not put the frame in the TracedValue returned by analyzerCounters()? It > already contains a hostname (which might become redundant if the frame could be > associated with the Frame object snapshots). Done.
On 2016/04/26 17:39:49, benjhayden_chromium wrote:
> +Sami, is it possible to correlate the FrameBlameContext with the LocalFrame
> pointer? It looks like there's a public/private layer boundary between them at
> the WebLocalFrame{,Impl}.
Not directly, no. What you could do instead is enter() and leave() the blame
context while serializing the counters so the object snapshot would have the
frame as its context.
On 2016/04/27 at 15:30:35, skyostil wrote:
> On 2016/04/26 17:39:49, benjhayden_chromium wrote:
> > +Sami, is it possible to correlate the FrameBlameContext with the LocalFrame
> > pointer? It looks like there's a public/private layer boundary between them
at
> > the WebLocalFrame{,Impl}.
>
> Not directly, no. What you could do instead is enter() and leave() the blame
context while serializing the counters so the object snapshot would have the
frame as its context.
Ah, I see. Hopefully the blame context will have already been entered by a
caller.
One nit then lgtm. These counters are perfect for benchmarks. Designing something more efficient when making a UMA sgtm. https://codereview.chromium.org/1922823003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1922823003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:842: value->setString("frame", String::format("0x%" PRIx64, static_cast<uint64_t>(reinterpret_cast<intptr_t>(m_frame.get())))); Yeah, sorry this is super non-obvious, but I'm told that the correct way to stringify pointers is like this: value->setString("frame", String::format("%" PRIxPTR, reinterpret_cast<uintptr_t>(m_frame.get())));
ksakamoto@chromium.org changed reviewers: + kinuko@chromium.org
kinuko@, could you do OWNER's review? https://codereview.chromium.org/1922823003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1922823003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:842: value->setString("frame", String::format("0x%" PRIx64, static_cast<uint64_t>(reinterpret_cast<intptr_t>(m_frame.get())))); On 2016/04/27 16:36:06, benjhayden_chromium wrote: > Yeah, sorry this is super non-obvious, but I'm told that the correct way to > stringify pointers is like this: > value->setString("frame", String::format("%" PRIxPTR, > reinterpret_cast<uintptr_t>(m_frame.get()))); Thanks for the tip, done. Let me keep the "0x" prefix for consistency with other trace events.
lgtm.
The CQ bit was checked by ksakamoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from benjhayden@chromium.org Link to the patchset: https://codereview.chromium.org/1922823003/#ps40001 (title: "PRIxPTR")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1922823003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1922823003/40001
https://codereview.chromium.org/1922823003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1922823003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:842: value->setString("frame", String::format("0x%" PRIx64, static_cast<uint64_t>(reinterpret_cast<intptr_t>(m_frame.get())))); On 2016/04/28 at 02:56:06, Kunihiko Sakamoto wrote: > On 2016/04/27 16:36:06, benjhayden_chromium wrote: > > Yeah, sorry this is super non-obvious, but I'm told that the correct way to > > stringify pointers is like this: > > value->setString("frame", String::format("%" PRIxPTR, > > reinterpret_cast<uintptr_t>(m_frame.get()))); > > Thanks for the tip, done. > Let me keep the "0x" prefix for consistency with other trace events. Doesn't the PRIxPTR already generate the 0x prefix?
https://codereview.chromium.org/1922823003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1922823003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:842: value->setString("frame", String::format("0x%" PRIx64, static_cast<uint64_t>(reinterpret_cast<intptr_t>(m_frame.get())))); On 2016/04/28 04:22:10, benjhayden_chromium wrote: > On 2016/04/28 at 02:56:06, Kunihiko Sakamoto wrote: > > On 2016/04/27 16:36:06, benjhayden_chromium wrote: > > > Yeah, sorry this is super non-obvious, but I'm told that the correct way to > > > stringify pointers is like this: > > > value->setString("frame", String::format("%" PRIxPTR, > > > reinterpret_cast<uintptr_t>(m_frame.get()))); > > > > Thanks for the tip, done. > > Let me keep the "0x" prefix for consistency with other trace events. > > Doesn't the PRIxPTR already generate the 0x prefix? No, on my Linux box, PRIxPTR is expanded to "lx" so 0x is not prepended.
Message was sent while issue was closed.
Description was changed from ========== Add frame ID to LayoutAnalyzer trace event This makes it easy to associate a LayoutAnalyzer event with other trace events. BUG=507790 ========== to ========== Add frame ID to LayoutAnalyzer trace event This makes it easy to associate a LayoutAnalyzer event with other trace events. BUG=507790 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6f19ad4e07796d9b62894e0b856b90ed0bd7649d Cr-Commit-Position: refs/heads/master@{#390306}
Message was sent while issue was closed.
Description was changed from ========== Add frame ID to LayoutAnalyzer trace event This makes it easy to associate a LayoutAnalyzer event with other trace events. BUG=507790 ========== to ========== Add frame ID to LayoutAnalyzer trace event This makes it easy to associate a LayoutAnalyzer event with other trace events. BUG=507790 Committed: https://crrev.com/6f19ad4e07796d9b62894e0b856b90ed0bd7649d Cr-Commit-Position: refs/heads/master@{#390306} ========== |
