Add trace event for detecting main frame in loading metrics.

Add trace event for detecting main frame in loading metrics.

The loading metrics use trace events with the "frame" argument to
compute the time to FCP, FMP, TTI. Example: https://goo.gl/U1qsqW.

In order to work properly the metrics need a way to filter out all
events that are not related to the main frame of the renderer.

This patch adds a trace event named "markAsMainFrame" in the "loading"
category with the "frame" argument equal to the local frame address in
- WebLocalFrameImpl::initializeCoreFrame,
- WebFrame::swap.

BUG=692112
Review-Url: https://codereview.chromium.org/2712773002
Cr-Commit-Position: refs/heads/master@{#454852}
Committed: https://chromium.googlesource.com/chromium/src/+/88feea0a921c2a57904b09c58f815e14f4def106

[ulan, 2017-02-22 20:53:10]

Description was changed from

==========
Add a trace event with the frame id in RenderFrameImpl::CreateMainFrame.

BUG=692112
==========

to

==========
Add trace event for loading metrics in RenderFrameImpl::CreateMainFrame.

The loading metrics use trace events with the "frame" argument to
compute time to first contentful paint, time to interactive, etc.
For example: https://goo.gl/U1qsqW

In order to work properly the metrics need a way to filter out all
events that are not related to the main frame of the renderer.
The loading events are emitted on the Blink side. At that point we
cannot tell if a frame is the main frame of the renderer or not.
We know if a frame is a top-level frame or not, but that does not
help (crbug.com/692112#c11).

This patch adds a trace event named "createMainFrame" in the "loading"
category with the "frame" argument equal to the local frame address.

BUG=692112
==========

[ulan, 2017-02-22 20:54:36]

Description was changed from

==========
Add trace event for loading metrics in RenderFrameImpl::CreateMainFrame.

The loading metrics use trace events with the "frame" argument to
compute time to first contentful paint, time to interactive, etc.
For example: https://goo.gl/U1qsqW

In order to work properly the metrics need a way to filter out all
events that are not related to the main frame of the renderer.
The loading events are emitted on the Blink side. At that point we
cannot tell if a frame is the main frame of the renderer or not.
We know if a frame is a top-level frame or not, but that does not
help (crbug.com/692112#c11).

This patch adds a trace event named "createMainFrame" in the "loading"
category with the "frame" argument equal to the local frame address.

BUG=692112
==========

to

==========
Add trace event for loading metrics in RenderFrameImpl::CreateMainFrame.

The loading metrics use trace events with the "frame" argument to
compute the time to FCP, FMP, TTI. Example: https://goo.gl/U1qsqW.

In order to work properly the metrics need a way to filter out all
events that are not related to the main frame of the renderer.
The loading events are emitted on the Blink side. At that point we
cannot tell if a frame is the main frame of the renderer or not.
We know if a frame is a top-level frame or not, but that does not
help (crbug.com/692112#c11).

This patch adds a trace event named "createMainFrame" in the "loading"
category with the "frame" argument equal to the local frame address.

BUG=692112
==========

[ulan, 2017-02-22 21:16:06]

ulan@chromium.org changed reviewers:
+ haraken@chromium.org, jochen@chromium.org

[ulan, 2017-02-22 21:16:37]

PTAL

https://codereview.chromium.org/2712773002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/2712773002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebLocalFrame.h:509: virtual void*
frameIdForTracing() const = 0;
This is not really nice. I would appreciate better suggestions.

The problem is that the existing TRACE_EVENTs use the address of the local frame
as the id (BTW, does it work if Oilpan does compaction?). 

The RenderFrameImpl doesn't seem to have access to the local frame, so we need
to either pull the frame id from Blink or push a bit to Blink saying whether the
frame is the main frame or not.

[haraken, 2017-02-22 23:56:21]

haraken@chromium.org changed reviewers:
+ dcheng@chromium.org

[haraken, 2017-02-22 23:56:21]

https://codereview.chromium.org/2712773002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/2712773002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebLocalFrame.h:509: virtual void*
frameIdForTracing() const = 0;
On 2017/02/22 21:16:37, ulan wrote:
> This is not really nice. I would appreciate better suggestions.
> 
> The problem is that the existing TRACE_EVENTs use the address of the local
frame
> as the id (BTW, does it work if Oilpan does compaction?). 
> 
> The RenderFrameImpl doesn't seem to have access to the local frame, so we need
> to either pull the frame id from Blink or push a bit to Blink saying whether
the
> frame is the main frame or not.

In practice, this will work because Oilpan's compaction moves only backing
stores of vectors, hashtables etc. Oilpan doesn't move normal objects like
Frame.

However, it's not nice to expose the frame ID via the public APIs.

+dcheng@ might have some ideas.

[dcheng, 2017-02-23 03:03:45]

On 2017/02/22 23:56:21, haraken wrote:
>
https://codereview.chromium.org/2712773002/diff/20001/third_party/WebKit/publ...
> File third_party/WebKit/public/web/WebLocalFrame.h (right):
> 
>
https://codereview.chromium.org/2712773002/diff/20001/third_party/WebKit/publ...
> third_party/WebKit/public/web/WebLocalFrame.h:509: virtual void*
> frameIdForTracing() const = 0;
> On 2017/02/22 21:16:37, ulan wrote:
> > This is not really nice. I would appreciate better suggestions.
> > 
> > The problem is that the existing TRACE_EVENTs use the address of the local
> frame
> > as the id (BTW, does it work if Oilpan does compaction?). 
> > 
> > The RenderFrameImpl doesn't seem to have access to the local frame, so we
need
> > to either pull the frame id from Blink or push a bit to Blink saying whether
> the
> > frame is the main frame or not.
> 
> In practice, this will work because Oilpan's compaction moves only backing
> stores of vectors, hashtables etc. Oilpan doesn't move normal objects like
> Frame.
> 
> However, it's not nice to expose the frame ID via the public APIs.
> 
> +dcheng@ might have some ideas.

Why does this tracing need to live in content instead of Blink?

[ulan, 2017-02-24 09:30:41]

On 2017/02/23 03:03:45, dcheng wrote:
> On 2017/02/22 23:56:21, haraken wrote:
> >
>
https://codereview.chromium.org/2712773002/diff/20001/third_party/WebKit/publ...
> > File third_party/WebKit/public/web/WebLocalFrame.h (right):
> > 
> >
>
https://codereview.chromium.org/2712773002/diff/20001/third_party/WebKit/publ...
> > third_party/WebKit/public/web/WebLocalFrame.h:509: virtual void*
> > frameIdForTracing() const = 0;
> > On 2017/02/22 21:16:37, ulan wrote:
> > > This is not really nice. I would appreciate better suggestions.
> > > 
> > > The problem is that the existing TRACE_EVENTs use the address of the local
> > frame
> > > as the id (BTW, does it work if Oilpan does compaction?). 
> > > 
> > > The RenderFrameImpl doesn't seem to have access to the local frame, so we
> need
> > > to either pull the frame id from Blink or push a bit to Blink saying
whether
> > the
> > > frame is the main frame or not.
> > 
> > In practice, this will work because Oilpan's compaction moves only backing
> > stores of vectors, hashtables etc. Oilpan doesn't move normal objects like
> > Frame.
> > 
> > However, it's not nice to expose the frame ID via the public APIs.
> > 
> > +dcheng@ might have some ideas.
> 
> Why does this tracing need to live in content instead of Blink?

In Blink we not know if a frame is the main frame of the renderer. (note that
there is isMainFrame predicate, but it just checks that the frame has no parent:
 https://goo.gl/U1qsqW).
Alternative to tracing in content would be to push "is_main_frame" bit to
Blink-frame from Renderer-frame.

[dcheng, 2017-02-24 18:04:19]

On 2017/02/24 09:30:41, ulan wrote:
> On 2017/02/23 03:03:45, dcheng wrote:
> > On 2017/02/22 23:56:21, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/2712773002/diff/20001/third_party/WebKit/publ...
> > > File third_party/WebKit/public/web/WebLocalFrame.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2712773002/diff/20001/third_party/WebKit/publ...
> > > third_party/WebKit/public/web/WebLocalFrame.h:509: virtual void*
> > > frameIdForTracing() const = 0;
> > > On 2017/02/22 21:16:37, ulan wrote:
> > > > This is not really nice. I would appreciate better suggestions.
> > > > 
> > > > The problem is that the existing TRACE_EVENTs use the address of the
local
> > > frame
> > > > as the id (BTW, does it work if Oilpan does compaction?). 
> > > > 
> > > > The RenderFrameImpl doesn't seem to have access to the local frame, so
we
> > need
> > > > to either pull the frame id from Blink or push a bit to Blink saying
> whether
> > > the
> > > > frame is the main frame or not.
> > > 
> > > In practice, this will work because Oilpan's compaction moves only backing
> > > stores of vectors, hashtables etc. Oilpan doesn't move normal objects like
> > > Frame.
> > > 
> > > However, it's not nice to expose the frame ID via the public APIs.
> > > 
> > > +dcheng@ might have some ideas.
> > 
> > Why does this tracing need to live in content instead of Blink?
> 
> In Blink we not know if a frame is the main frame of the renderer. (note that
> there is isMainFrame predicate, but it just checks that the frame has no
parent:
>  https://goo.gl/U1qsqW).
> Alternative to tracing in content would be to push "is_main_frame" bit to
> Blink-frame from Renderer-frame.

Why isn't isMainFrame() reliable enough, especially during frame load, when the
frame is attached to the frame tree?

[ulan, 2017-02-24 18:13:53]

On 2017/02/24 18:04:19, dcheng wrote:
> On 2017/02/24 09:30:41, ulan wrote:
> > On 2017/02/23 03:03:45, dcheng wrote:
> > > On 2017/02/22 23:56:21, haraken wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2712773002/diff/20001/third_party/WebKit/publ...
> > > > File third_party/WebKit/public/web/WebLocalFrame.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2712773002/diff/20001/third_party/WebKit/publ...
> > > > third_party/WebKit/public/web/WebLocalFrame.h:509: virtual void*
> > > > frameIdForTracing() const = 0;
> > > > On 2017/02/22 21:16:37, ulan wrote:
> > > > > This is not really nice. I would appreciate better suggestions.
> > > > > 
> > > > > The problem is that the existing TRACE_EVENTs use the address of the
> local
> > > > frame
> > > > > as the id (BTW, does it work if Oilpan does compaction?). 
> > > > > 
> > > > > The RenderFrameImpl doesn't seem to have access to the local frame, so
> we
> > > need
> > > > > to either pull the frame id from Blink or push a bit to Blink saying
> > whether
> > > > the
> > > > > frame is the main frame or not.
> > > > 
> > > > In practice, this will work because Oilpan's compaction moves only
backing
> > > > stores of vectors, hashtables etc. Oilpan doesn't move normal objects
like
> > > > Frame.
> > > > 
> > > > However, it's not nice to expose the frame ID via the public APIs.
> > > > 
> > > > +dcheng@ might have some ideas.
> > > 
> > > Why does this tracing need to live in content instead of Blink?
> > 
> > In Blink we not know if a frame is the main frame of the renderer. (note
that
> > there is isMainFrame predicate, but it just checks that the frame has no
> parent:
> >  https://goo.gl/U1qsqW).
> > Alternative to tracing in content would be to push "is_main_frame" bit to
> > Blink-frame from Renderer-frame.
> 
> Why isn't isMainFrame() reliable enough, especially during frame load, when
the
> frame is attached to the frame tree?

Sorry, I meant to paste this link above: https://crbug.com/692112#c11
The problem is other top-level frames returning isMainFrame()=true even though
they are not the main frame of the renderer.

[dcheng, 2017-02-24 18:28:27]

On 2017/02/24 18:13:53, ulan wrote:
> On 2017/02/24 18:04:19, dcheng wrote:
> > On 2017/02/24 09:30:41, ulan wrote:
> > > On 2017/02/23 03:03:45, dcheng wrote:
> > > > On 2017/02/22 23:56:21, haraken wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2712773002/diff/20001/third_party/WebKit/publ...
> > > > > File third_party/WebKit/public/web/WebLocalFrame.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2712773002/diff/20001/third_party/WebKit/publ...
> > > > > third_party/WebKit/public/web/WebLocalFrame.h:509: virtual void*
> > > > > frameIdForTracing() const = 0;
> > > > > On 2017/02/22 21:16:37, ulan wrote:
> > > > > > This is not really nice. I would appreciate better suggestions.
> > > > > > 
> > > > > > The problem is that the existing TRACE_EVENTs use the address of the
> > local
> > > > > frame
> > > > > > as the id (BTW, does it work if Oilpan does compaction?). 
> > > > > > 
> > > > > > The RenderFrameImpl doesn't seem to have access to the local frame,
so
> > we
> > > > need
> > > > > > to either pull the frame id from Blink or push a bit to Blink saying
> > > whether
> > > > > the
> > > > > > frame is the main frame or not.
> > > > > 
> > > > > In practice, this will work because Oilpan's compaction moves only
> backing
> > > > > stores of vectors, hashtables etc. Oilpan doesn't move normal objects
> like
> > > > > Frame.
> > > > > 
> > > > > However, it's not nice to expose the frame ID via the public APIs.
> > > > > 
> > > > > +dcheng@ might have some ideas.
> > > > 
> > > > Why does this tracing need to live in content instead of Blink?
> > > 
> > > In Blink we not know if a frame is the main frame of the renderer. (note
> that
> > > there is isMainFrame predicate, but it just checks that the frame has no
> > parent:
> > >  https://goo.gl/U1qsqW).
> > > Alternative to tracing in content would be to push "is_main_frame" bit to
> > > Blink-frame from Renderer-frame.
> > 
> > Why isn't isMainFrame() reliable enough, especially during frame load, when
> the
> > frame is attached to the frame tree?
> 
> Sorry, I meant to paste this link above: https://crbug.com/692112#c11
> The problem is other top-level frames returning isMainFrame()=true even though
> they are not the main frame of the renderer.

I guess I'm confused--if we're printing out the frame ID in Blink, shouldn't we
be able to use that to disambiguate?

[ulan, 2017-02-24 18:46:51]

> I guess I'm confused--if we're printing out the frame ID in Blink, shouldn't
we
> be able to use that to disambiguate?
Blink prints frame ID and even prints "isLoadingMainFrame"
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr...)

The problem is that isLoadingMainFrame == frame->isMainFrame() == 
!frame->tree().parent().
That is isLoadingMainFrame cannot be trusted as returns true for frames that are
not really the main frame (they are just the top-frames)
Example from trace:
frameId: 0x389423d33d40, isLoadingMainFrame: true, url: about:blank
frameId: 0x10d91bd81dd0, isLoadingMainFrame: true, url: http://www.nytimes.com/

In this patch I am trying to add a line to trace that says:
frameId: 0x10d91bd81dd0, createMainFrame

With that I can see that 0x10d91bd81dd0 is the main frame and 0x389423d33d40 is
not the main frame.

[dcheng, 2017-02-25 01:37:16]

On 2017/02/24 18:46:51, ulan wrote:
> > I guess I'm confused--if we're printing out the frame ID in Blink, shouldn't
> we
> > be able to use that to disambiguate?
> Blink prints frame ID and even prints "isLoadingMainFrame"
>
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr...)
> 
> The problem is that isLoadingMainFrame == frame->isMainFrame() == 
> !frame->tree().parent().
> That is isLoadingMainFrame cannot be trusted as returns true for frames that
are
> not really the main frame (they are just the top-frames)
> Example from trace:
> frameId: 0x389423d33d40, isLoadingMainFrame: true, url: about:blank
> frameId: 0x10d91bd81dd0, isLoadingMainFrame: true, url:
http://www.nytimes.com/
> 
> In this patch I am trying to add a line to trace that says:
> frameId: 0x10d91bd81dd0, createMainFrame
> 
> With that I can see that 0x10d91bd81dd0 is the main frame and 0x389423d33d40
is
> not the main frame.

OK, so I think you should be hooking into Page::setMainFrame() instead. This
will allow accurate tracking, even when the main frame is swapped for OOPIF.

[ulan, 2017-02-27 15:45:22]

> OK, so I think you should be hooking into Page::setMainFrame() instead. This
> will allow accurate tracking, even when the main frame is swapped for OOPIF.

Thank you for the suggestion. Unfortunately, Page::setMainFrame() is also called
for bunch of frames that are not the renderer main frame.
For example, a frame created for SVG:
#0 0x7f6fd969355b base::debug::StackTrace::StackTrace()
#1 0x7f6fd969214c base::debug::StackTrace::StackTrace()
#2 0x7f6fe02c4db4 blink::Page::setMainFrame()
#3 0x7f6fdfadb48e blink::Frame::Frame()
#4 0x7f6fdfb453f8 blink::LocalFrame::LocalFrame()
#5 0x7f6fdfb3fc52 blink::LocalFrame::create()
#6 0x7f6fe04d9b1a blink::SVGImage::dataChanged()
#7 0x7f6fdec31adc blink::Image::setData()
#8 0x7f6fe028eb28 blink::ImageResourceContent::updateImage()
#9 0x7f6fe028908c blink::ImageResource::updateImage()

I think with Page::setMainFrame() I am getting the same (incorrect) set of
frames as with frame->isMainFrame().

[dcheng, 2017-02-28 06:28:07]

On 2017/02/27 15:45:22, ulan wrote:
> > OK, so I think you should be hooking into Page::setMainFrame() instead. This
> > will allow accurate tracking, even when the main frame is swapped for OOPIF.
> 
> Thank you for the suggestion. Unfortunately, Page::setMainFrame() is also
called
> for bunch of frames that are not the renderer main frame.
> For example, a frame created for SVG:
> #0 0x7f6fd969355b base::debug::StackTrace::StackTrace()
> #1 0x7f6fd969214c base::debug::StackTrace::StackTrace()
> #2 0x7f6fe02c4db4 blink::Page::setMainFrame()
> #3 0x7f6fdfadb48e blink::Frame::Frame()
> #4 0x7f6fdfb453f8 blink::LocalFrame::LocalFrame()
> #5 0x7f6fdfb3fc52 blink::LocalFrame::create()
> #6 0x7f6fe04d9b1a blink::SVGImage::dataChanged()
> #7 0x7f6fdec31adc blink::Image::setData()
> #8 0x7f6fe028eb28 blink::ImageResourceContent::updateImage()
> #9 0x7f6fe028908c blink::ImageResource::updateImage()
> 
> I think with Page::setMainFrame() I am getting the same (incorrect) set of
> frames as with frame->isMainFrame().

Ah, right--you need a hook in the web layer, not in core. You can get what you
want by emitting the trace in two places:
- WebLocalFrameImpl::initializeCoreFrame: if |owner| is null
- WebFrame::swap: at
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebFrame.c...

This has the bonus of handling OOPIF frame swaps, which this CL currently
doesn't.

[ulan, 2017-02-28 14:50:20]

Description was changed from

==========
Add trace event for loading metrics in RenderFrameImpl::CreateMainFrame.

The loading metrics use trace events with the "frame" argument to
compute the time to FCP, FMP, TTI. Example: https://goo.gl/U1qsqW.

In order to work properly the metrics need a way to filter out all
events that are not related to the main frame of the renderer.
The loading events are emitted on the Blink side. At that point we
cannot tell if a frame is the main frame of the renderer or not.
We know if a frame is a top-level frame or not, but that does not
help (crbug.com/692112#c11).

This patch adds a trace event named "createMainFrame" in the "loading"
category with the "frame" argument equal to the local frame address.

BUG=692112
==========

to

==========
Add trace event for detecting main frame in loading metrics.

The loading metrics use trace events with the "frame" argument to
compute the time to FCP, FMP, TTI. Example: https://goo.gl/U1qsqW.

In order to work properly the metrics need a way to filter out all
events that are not related to the main frame of the renderer.

This patch adds a trace event named "markAsMainFrame" in the "loading"
category with the "frame" argument equal to the local frame address in
- WebLocalFrameImpl::initializeCoreFrame,
- WebFrame::swap.

BUG=692112
==========

[ulan, 2017-02-28 14:55:44]

Thank you! PTAL at the latest patch set.

This filters out SVG frames. I still get frames from plugins (stack trace
below).
If you have idea how to exclude these frame, please let me know.
Otherwise, we can land the CL and I can filter out plugin frames on telemetry
side using the url.

#0 0x7ff068d8145b base::debug::StackTrace::StackTrace()
#1 0x7ff068d8009c base::debug::StackTrace::StackTrace()
#2 0x7ff06e4154a9 blink::WebLocalFrameImpl::initializeCoreFrame()
#3 0x7ff06e442546 blink::WebViewImpl::setMainFrame()
#4 0x7ff070e48a6c WebViewPlugin::WebViewHelper::WebViewHelper()
#5 0x7ff070e471c9 WebViewPlugin::WebViewPlugin()
#6 0x7ff070e47307 WebViewPlugin::Create()
#7 0x7ff070e45445 plugins::PluginPlaceholderBase::PluginPlaceholderBase()
#8 0x7ff070e4a3dd
plugins::LoadablePluginPlaceholder::LoadablePluginPlaceholder()
#9 0x7ff0706ad7a1 ChromePluginPlaceholder::ChromePluginPlaceholder()
#10 0x7ff0706adced ChromePluginPlaceholder::CreateLoadableMissingPlugin()

[dcheng, 2017-03-01 00:16:29]

On 2017/02/28 14:55:44, ulan wrote:
> Thank you! PTAL at the latest patch set.
> 
> This filters out SVG frames. I still get frames from plugins (stack trace
> below).
> If you have idea how to exclude these frame, please let me know.
> Otherwise, we can land the CL and I can filter out plugin frames on telemetry
> side using the url.
> 
> #0 0x7ff068d8145b base::debug::StackTrace::StackTrace()
> #1 0x7ff068d8009c base::debug::StackTrace::StackTrace()
> #2 0x7ff06e4154a9 blink::WebLocalFrameImpl::initializeCoreFrame()
> #3 0x7ff06e442546 blink::WebViewImpl::setMainFrame()
> #4 0x7ff070e48a6c WebViewPlugin::WebViewHelper::WebViewHelper()
> #5 0x7ff070e471c9 WebViewPlugin::WebViewPlugin()
> #6 0x7ff070e47307 WebViewPlugin::Create()
> #7 0x7ff070e45445 plugins::PluginPlaceholderBase::PluginPlaceholderBase()
> #8 0x7ff070e4a3dd
> plugins::LoadablePluginPlaceholder::LoadablePluginPlaceholder()
> #9 0x7ff0706ad7a1 ChromePluginPlaceholder::ChromePluginPlaceholder()
> #10 0x7ff0706adced ChromePluginPlaceholder::CreateLoadableMissingPlugin()

How hard is it to filter out plugin frames? If it's not hard, that's probably
the easiest approach for now.

[dcheng, 2017-03-01 01:16:45]

also, i'm curious how tracing by frame pointer will work with oilpan compaction,
since pointer values won't be stable, right?

[haraken, 2017-03-01 01:20:19]

On 2017/03/01 01:16:45, dcheng wrote:
> also, i'm curious how tracing by frame pointer will work with oilpan
compaction,
> since pointer values won't be stable, right?

Oilpan compaction works only on backing buffers of vectors and hash tables (at
least currently). It doesn't move normal C++ objects.

[dcheng, 2017-03-01 01:30:41]

On 2017/03/01 01:20:19, haraken wrote:
> On 2017/03/01 01:16:45, dcheng wrote:
> > also, i'm curious how tracing by frame pointer will work with oilpan
> compaction,
> > since pointer values won't be stable, right?
> 
> Oilpan compaction works only on backing buffers of vectors and hash tables (at
> least currently). It doesn't move normal C++ objects.

I see. Will that change at some point in the future?

Also... do we emit a trace message when a frame is detached? I'm wondering if we
need to worry about a new Frame being allocated at that address.

[haraken, 2017-03-01 01:35:28]

On 2017/03/01 01:30:41, dcheng wrote:
> On 2017/03/01 01:20:19, haraken wrote:
> > On 2017/03/01 01:16:45, dcheng wrote:
> > > also, i'm curious how tracing by frame pointer will work with oilpan
> > compaction,
> > > since pointer values won't be stable, right?
> > 
> > Oilpan compaction works only on backing buffers of vectors and hash tables
(at
> > least currently). It doesn't move normal C++ objects.
> 
> I see. Will that change at some point in the future?

Yes, it's not nice to write code that relies on a pointer address of Oilpan's
object. However, it's a long-term thing so I'm not planning to block people from
using that if the pointer address provides a good solution at the moment.

[ulan, 2017-03-01 10:02:42]

> How hard is it to filter out plugin frames?
It shouldn't be hard since plugin frames have special url. Let's do this part in
telemetry.

> Also... do we emit a trace message when a frame is detached?
I think we do:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr...

Does the cl look good?

[dcheng, 2017-03-01 21:45:25]

On 2017/03/01 10:02:42, ulan wrote:
> > How hard is it to filter out plugin frames?
> It shouldn't be hard since plugin frames have special url. Let's do this part
in
> telemetry.
> 
> > Also... do we emit a trace message when a frame is detached?
> I think we do:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr...
> 
> Does the cl look good?

OK, that thing looks like it's using the FrameLoader pointer for identity,
rather than the LocalFrame though.
If this isn't an issue, and we correctly clear the main frame association on
detach (which can happen when frames are swapped), then this CL lgtm

[haraken, 2017-03-02 01:01:59]

On 2017/03/01 21:45:25, dcheng wrote:
> On 2017/03/01 10:02:42, ulan wrote:
> > > How hard is it to filter out plugin frames?
> > It shouldn't be hard since plugin frames have special url. Let's do this
part
> in
> > telemetry.
> > 
> > > Also... do we emit a trace message when a frame is detached?
> > I think we do:
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr...
> > 
> > Does the cl look good?
> 
> OK, that thing looks like it's using the FrameLoader pointer for identity,
> rather than the LocalFrame though.
> If this isn't an issue, and we correctly clear the main frame association on
> detach (which can happen when frames are swapped), then this CL lgtm

Shall we add a comment about it to the code?

LGTM.

[ulan, 2017-03-06 10:23:42]

> OK, that thing looks like it's using the FrameLoader pointer for identity,
> rather than the LocalFrame though.
> If this isn't an issue, and we correctly clear the main frame association on
> detach (which can happen when frames are swapped), then this CL lgtm
I checked locally that for each 'MarkAsMainFrame' event in the trace we can find
the corresponding FrameLoader instance based on the create/detach timestamps
(similar to
https://cs.chromium.org/chromium/src/third_party/catapult/tracing/tracing/met...)

> Shall we add a comment about it to the code?
The main logic will be in the catapult code. I'll put comments there.
FrameLoader itself already produces trace events that use the frame as identity:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr...
So we are not introducing new way of identifying frames.

[ulan, 2017-03-06 10:24:45]

Thanks all, landing.

[ulan, 2017-03-06 10:24:54]

The CQ bit was checked by ulan@chromium.org

[commit-bot: I haz the power, 2017-03-06 10:25:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 11:46:26]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488795894609860,
"parent_rev": "04af4448e4833d6d1e9e06059ab9947a4d936111", "commit_rev":
"88feea0a921c2a57904b09c58f815e14f4def106"}

[commit-bot: I haz the power, 2017-03-06 11:47:03]

Description was changed from

==========
Add trace event for detecting main frame in loading metrics.

The loading metrics use trace events with the "frame" argument to
compute the time to FCP, FMP, TTI. Example: https://goo.gl/U1qsqW.

In order to work properly the metrics need a way to filter out all
events that are not related to the main frame of the renderer.

This patch adds a trace event named "markAsMainFrame" in the "loading"
category with the "frame" argument equal to the local frame address in
- WebLocalFrameImpl::initializeCoreFrame,
- WebFrame::swap.

BUG=692112
==========

to

==========
Add trace event for detecting main frame in loading metrics.

The loading metrics use trace events with the "frame" argument to
compute the time to FCP, FMP, TTI. Example: https://goo.gl/U1qsqW.

In order to work properly the metrics need a way to filter out all
events that are not related to the main frame of the renderer.

This patch adds a trace event named "markAsMainFrame" in the "loading"
category with the "frame" argument equal to the local frame address in
- WebLocalFrameImpl::initializeCoreFrame,
- WebFrame::swap.

BUG=692112

Review-Url: https://codereview.chromium.org/2712773002
Cr-Commit-Position: refs/heads/master@{#454852}
Committed:
https://chromium.googlesource.com/chromium/src/+/88feea0a921c2a57904b09c58f81...
==========

[commit-bot: I haz the power, 2017-03-06 11:47:04]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/88feea0a921c2a57904b09c58f81...