Attribute Stuff to Frames with FrameBlamer

Attribute Stuff to Frames with FrameBlamer

This patch attributes the cost of the following functions to individual frames:
- FrameView::updateStyleAndLayoutIfNeededRecursiveInternal
- PageAnimator::serviceScriptedAnimations
- V8ScriptRunner::callFunction

The above attribution is built on a new helper class ScopedFrameBlamer, a
wrapper of a LocalFrame's frame blame context, for automatically calling the
blame context's Enter() and Leave() functions when we enter/exit a scope.

Design doc: https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8APtI/edit?usp=sharing

BUG=546021
Committed: https://crrev.com/bb41bc42d96c37688310a8e8afe9d92960d53a25
Cr-Commit-Position: refs/heads/master@{#397984}

[Xiaocheng, 2016-06-01 08:29:53]

Description was changed from

==========
Attribute Script Running to Frames

This patch attributes the cost of the following functions to the frames
where the scripts are from:
- V8ScriptRunner::runCompiledScript
- V8ScriptRunner::callFunction
- V8ScriptRunner::instantiateObjectInDocument

BUG=546021
==========

to

==========
Attribute Stuff to Frames

This patch attributes the cost of the following functions to frames:
- FrameView::updateStyleAndLayoutIfNeededRecursiveInternal
- PageAnimator::serviceScriptedAnimations
- V8ScriptRunner::callFunction

BUG=546021
==========

[Xiaocheng, 2016-06-01 09:34:24]

Description was changed from

==========
Attribute Stuff to Frames

This patch attributes the cost of the following functions to frames:
- FrameView::updateStyleAndLayoutIfNeededRecursiveInternal
- PageAnimator::serviceScriptedAnimations
- V8ScriptRunner::callFunction

BUG=546021
==========

to

==========
Attribute Stuff to Frames with FrameBlamer

This patch attributes the cost of the following functions to individual frames:
- FrameView::updateStyleAndLayoutIfNeededRecursiveInternal
- PageAnimator::serviceScriptedAnimations
- V8ScriptRunner::callFunction

Design doc:
https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8AP...

BUG=546021
==========

[Xiaocheng, 2016-06-01 09:34:24]

xiaochengh@chromium.org changed reviewers:
+ skyostil@chromium.org, tkent@chromium.org, yhirano@chromium.org

[Xiaocheng, 2016-06-01 09:36:33]

Description was changed from

==========
Attribute Stuff to Frames with FrameBlamer

This patch attributes the cost of the following functions to individual frames:
- FrameView::updateStyleAndLayoutIfNeededRecursiveInternal
- PageAnimator::serviceScriptedAnimations
- V8ScriptRunner::callFunction

Design doc:
https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8AP...

BUG=546021
==========

to

==========
[ALL IN ONE] Attribute Stuff to Frames with FrameBlamer

This patch attributes the cost of the following functions to individual frames:
- FrameView::updateStyleAndLayoutIfNeededRecursiveInternal
- PageAnimator::serviceScriptedAnimations
- V8ScriptRunner::callFunction

Design doc:
https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8AP...

BUG=546021
==========

[Xiaocheng, 2016-06-01 09:45:27]

Description was changed from

==========
[ALL IN ONE] Attribute Stuff to Frames with FrameBlamer

This patch attributes the cost of the following functions to individual frames:
- FrameView::updateStyleAndLayoutIfNeededRecursiveInternal
- PageAnimator::serviceScriptedAnimations
- V8ScriptRunner::callFunction

Design doc:
https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8AP...

BUG=546021
==========

to

==========
[ALL IN ONE] Attribute Stuff to Frames with FrameBlamer

This patch attributes the cost of the following functions to individual frames:
- FrameView::updateStyleAndLayoutIfNeededRecursiveInternal
- PageAnimator::serviceScriptedAnimations
- V8ScriptRunner::callFunction

The above attribution is built on a new helper class ScopedFrameBlamer, a
wrapper of a LocalFrame's frame blame context, for automatically calling the
blame context's Enter() and Leave() functions when we enter/exit a scope.

Design doc:
https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8AP...

BUG=546021
==========

[Xiaocheng, 2016-06-01 09:47:59]

PTAL.

Should I break this patch into 4 smaller ones (the helper class and its 3
applications) to commit?

[Sami, 2016-06-01 11:05:17]

lgtm. This doesn't seem too large to me to land in one go.

https://codereview.chromium.org/2028523002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/2028523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.h:322: explicit
ScopedFrameBlamer(LocalFrame*);
Could the parameter be const?

[yhirano, 2016-06-01 12:26:05]

https://codereview.chromium.org/2028523002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/2028523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:448:
ScopedFrameBlamer frameBlamer(context->isDocument() ?
toDocument(context)->frame() : nullptr);
Do you want to put the frame blamer only in callFunction or in other functions
(such as callInternalFunction, runCompiledScript) as well? (I'm not suggesting
to do that, just asking.)

[tkent, 2016-06-01 23:14:56]

https://codereview.chromium.org/2028523002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/2028523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.h:318: class ScopedFrameBlamer {
Please add a comment that what this class does concretely.
I couldn't guess what's "FrameBlamer" without reading the design doc.

https://codereview.chromium.org/2028523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.h:326: BlameContext*
m_frameBlameContext;
Is it safe to hold a raw pointer? Can the BlameContext object die during a
ScopedFrameBlamer is alive?

[Xiaocheng, 2016-06-02 03:17:20]

Thanks for the review. 

There is a concern that can |m_frameBlameContext| get deleted when a
|ScopedFrameBlamer| is still alive. |m_frameBlameContext| is owned by the
Chromium-side's RenderFrameImpl corresponding to the LocalFrame (*), so the
question is about whether the RenderFrameImpl is always alive. Can I safely
assume that:

1. Everything this patch touches runs on the main thread

2. A Chromium's object cannot get deleted when we are running Blink's code (on
the main thread)

If so, then |m_frameBlameContext| is always alive and it's safe to store a raw
ptr there.

https://codereview.chromium.org/2028523002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/2028523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:448:
ScopedFrameBlamer frameBlamer(context->isDocument() ?
toDocument(context)->frame() : nullptr);
On 2016/06/01 at 12:26:05, yhirano wrote:
> Do you want to put the frame blamer only in callFunction or in other functions
(such as callInternalFunction, runCompiledScript) as well? (I'm not suggesting
to do that, just asking.)

Nope.

https://codereview.chromium.org/2028523002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/2028523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.h:318: class ScopedFrameBlamer {
On 2016/06/01 at 23:14:56, tkent wrote:
> Please add a comment that what this class does concretely.
> I couldn't guess what's "FrameBlamer" without reading the design doc.

Done.

https://codereview.chromium.org/2028523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.h:322: explicit
ScopedFrameBlamer(LocalFrame*);
On 2016/06/01 at 11:05:17, Sami wrote:
> Could the parameter be const?

Yes. Done.

https://codereview.chromium.org/2028523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.h:326: BlameContext*
m_frameBlameContext;
On 2016/06/01 at 23:14:56, tkent wrote:
> Is it safe to hold a raw pointer? Can the BlameContext object die during a
ScopedFrameBlamer is alive?

FrameBlameContext (the BlameContext subclass for frames) objects are created and
owned (*) by content::RenderFrameImpls. I guess a RenderFrameImpl won't get
deleted as long as the corresponding LocalFrame is alive, right?

So it seems that we need to make sure |frame| is alive during the lifetime of a
ScopedFrameBlamer, which means we need to store |frame| as a
|Member<LocalFrame>| here. But the overhead maybe be too big since
ScopedFrameBlamer is supposed to be a helper class for tracing that may appear
anywhere.

(*) In the current implementation, FrameBlameContext objects never get deleted.
It's a memory leak that needs to be fixed. It also means we don't need to worry
about the lifetime of |m_frameBlameContext| before the fix...

[Xiaocheng, 2016-06-02 03:30:59]

(Please ignore the previous message...)

Thanks for the review. 

There is a concern that can |m_frameBlameContext| get deleted when a
|ScopedFrameBlamer| is still alive. |m_frameBlameContext| is owned by the
Chromium-side's RenderFrameImpl corresponding to the LocalFrame (*), so the
question is about whether the RenderFrameImpl is always alive. Can I safely
assume that:

1. Everything this patch touches runs on the main thread

2. A Chromium's object cannot get deleted when we are running Blink's code (on
the main thread)

If so, then |m_frameBlameContext| is always alive and it's safe to store a raw
ptr there.

(*) Currently RenderFrameImpls don't delete their blame contexts, which is a
memory leak to be fixed. We don't need to worry about the deletion of
|m_frameBlameContext| in this case, either...

https://codereview.chromium.org/2028523002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/2028523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:448:
ScopedFrameBlamer frameBlamer(context->isDocument() ?
toDocument(context)->frame() : nullptr);
On 2016/06/01 at 12:26:05, yhirano wrote:
> Do you want to put the frame blamer only in callFunction or in other functions
(such as callInternalFunction, runCompiledScript) as well? (I'm not suggesting
to do that, just asking.)

Nope.

Can I assume that when |context->isDocument()| is true, it is the main thread?

If not, the patch needs to handle this case.

https://codereview.chromium.org/2028523002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/2028523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.h:318: class ScopedFrameBlamer {
On 2016/06/01 at 23:14:56, tkent wrote:
> Please add a comment that what this class does concretely.
> I couldn't guess what's "FrameBlamer" without reading the design doc.

Done.

https://codereview.chromium.org/2028523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.h:322: explicit
ScopedFrameBlamer(LocalFrame*);
On 2016/06/01 at 11:05:17, Sami wrote:
> Could the parameter be const?

Yes. Done.

https://codereview.chromium.org/2028523002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.h:326: BlameContext*
m_frameBlameContext;
On 2016/06/01 at 23:14:56, tkent wrote:
> Is it safe to hold a raw pointer? Can the BlameContext object die during a
ScopedFrameBlamer is alive?

See the comment at the beginning of this message.

[tkent, 2016-06-02 03:37:51]

On 2016/06/02 at 03:30:59, xiaochengh wrote:
> (Please ignore the previous message...)
> 
> Thanks for the review. 
> 
> There is a concern that can |m_frameBlameContext| get deleted when a
|ScopedFrameBlamer| is still alive. |m_frameBlameContext| is owned by the
Chromium-side's RenderFrameImpl corresponding to the LocalFrame (*), so the
question is about whether the RenderFrameImpl is always alive. Can I safely
assume that:
> 
> 1. Everything this patch touches runs on the main thread

Yes.

> 2. A Chromium's object cannot get deleted when we are running Blink's code (on
the main thread)

In general, it can.
If a JavaScript code called in V8ScriptRunner::callFunction() triggers
Frame::detach(), the frame can be deleted.  It might delete RenderFrameImpl
though I don't take a look at the code.

[Xiaocheng, 2016-06-02 05:00:29]

On 2016/06/02 at 03:37:51, tkent wrote:
> On 2016/06/02 at 03:30:59, xiaochengh wrote:
> > (Please ignore the previous message...)
> > 
> > Thanks for the review. 
> > 
> > There is a concern that can |m_frameBlameContext| get deleted when a
|ScopedFrameBlamer| is still alive. |m_frameBlameContext| is owned by the
Chromium-side's RenderFrameImpl corresponding to the LocalFrame (*), so the
question is about whether the RenderFrameImpl is always alive. Can I safely
assume that:
> > 
> > 1. Everything this patch touches runs on the main thread
> 
> Yes.
> 
> > 2. A Chromium's object cannot get deleted when we are running Blink's code
(on the main thread)
> 
> In general, it can.
> If a JavaScript code called in V8ScriptRunner::callFunction() triggers
Frame::detach(), the frame can be deleted.  It might delete RenderFrameImpl
though I don't take a look at the code.

RenderFrameImpl can indeed be synchronously deleted from Blink code:
1. Frame::detach() calls FrameClient::detached()
2. FrameLoaderClientImpl::detached() overrides FrameClient::detached(), and
calls WebFrameClient::frameDetached()
3. RenderFrameImpl::frameDetached() overrides WebFrameClient::frameDetached(),
and calls destructor on itself

This patch needs some revision, at least for the part in V8ScriptRunner.

Still, can I assume that no frame can be detached in
FrameView::updateStyleAndLayoutIfNeededRecursiveInternal or
PageAnimator::serviceScriptedAnimations? They look quite harmless.

[tkent, 2016-06-02 06:30:46]

> Still, can I assume that no frame can be detached in
FrameView::updateStyleAndLayoutIfNeededRecursiveInternal or
PageAnimator::serviceScriptedAnimations? They look quite harmless.

I think so. It seems they don't dispatch events which run arbitrary JavaScript
code.

[Xiaocheng, 2016-06-02 08:58:11]

Some thoughts on using FrameBlameContext in Blink. Maybe we should let
LocalFrame store and own a duplicate of (a Blink variant of) FrameBlameContext,
instead of jumping out of Blink and grab it from RenderFrameImpl?

The current design looks fundamentally unsafe to me. LocalFrame is inside Blink
and has its lifetime managed by Oilpan. However, FrameBlameContext is outside
Blink and (ideally) owned by RenderFrameImpl, which is immediately gone when
Frame::detach() is called. In other words, we have to manually check if the
RenderFrameImpl will be gone before even trying to attribute some work, which is
not good.

For the same reason, the cost V8ScriptRunner::callFunction() cannot be
attributed using a FrameBlameContext, because Frame::detach() can be triggered
in Javascript.

Of course, we can rely on the current memory leak that FrameBlameContexts never
get deleted. It doesn't seem to be a good idea to me, though, unless we want to
call this bug a feature that won't be fixed.

[Sami, 2016-06-02 09:39:38]

On 2016/06/02 08:58:11, Xiaocheng wrote:
> Some thoughts on using FrameBlameContext in Blink. Maybe we should let
> LocalFrame store and own a duplicate of (a Blink variant of)
FrameBlameContext,
> instead of jumping out of Blink and grab it from RenderFrameImpl?
> 
> The current design looks fundamentally unsafe to me. LocalFrame is inside
Blink
> and has its lifetime managed by Oilpan. However, FrameBlameContext is outside
> Blink and (ideally) owned by RenderFrameImpl, which is immediately gone when
> Frame::detach() is called. In other words, we have to manually check if the
> RenderFrameImpl will be gone before even trying to attribute some work, which
is
> not good.
> 
> For the same reason, the cost V8ScriptRunner::callFunction() cannot be
> attributed using a FrameBlameContext, because Frame::detach() can be triggered
> in Javascript.
> 
> Of course, we can rely on the current memory leak that FrameBlameContexts
never
> get deleted. It doesn't seem to be a good idea to me, though, unless we want
to
> call this bug a feature that won't be fixed.

Yeah, I wouldn't like to rely on a memory leak.

Instead of storing a reference to the BlameContext, could be instead keep a
reference to an Oilpan object like the Document? That way we can safely check if
the frame was detached in the middle of processing.

By the way, PageAnimator::serviceScriptedAnimations does call arbitrary
Javascript in the form of requestAnimationFrame callbacks.

[Xiaocheng, 2016-06-02 13:20:46]

On 2016/06/02 at 09:39:38, skyostil wrote:
> On 2016/06/02 08:58:11, Xiaocheng wrote:
> > Some thoughts on using FrameBlameContext in Blink. Maybe we should let
> > LocalFrame store and own a duplicate of (a Blink variant of)
FrameBlameContext,
> > instead of jumping out of Blink and grab it from RenderFrameImpl?
> > 
> > The current design looks fundamentally unsafe to me. LocalFrame is inside
Blink
> > and has its lifetime managed by Oilpan. However, FrameBlameContext is
outside
> > Blink and (ideally) owned by RenderFrameImpl, which is immediately gone when
> > Frame::detach() is called. In other words, we have to manually check if the
> > RenderFrameImpl will be gone before even trying to attribute some work,
which is
> > not good.
> > 
> > For the same reason, the cost V8ScriptRunner::callFunction() cannot be
> > attributed using a FrameBlameContext, because Frame::detach() can be
triggered
> > in Javascript.
> > 
> > Of course, we can rely on the current memory leak that FrameBlameContexts
never
> > get deleted. It doesn't seem to be a good idea to me, though, unless we want
to
> > call this bug a feature that won't be fixed.
> 
> Yeah, I wouldn't like to rely on a memory leak.
> 
> Instead of storing a reference to the BlameContext, could be instead keep a
reference to an Oilpan object like the Document? That way we can safely check if
the frame was detached in the middle of processing.

The problem is that we must call BlameContext::Leave() at the end, which means
that the BlameContext must not be deleted in the middle of the attributed work.

So it seems that at the beginning, we should not call BlameContext::Enter() on
the FrameBlameContext itself, but a duplicate of it.

> 
> By the way, PageAnimator::serviceScriptedAnimations does call arbitrary
Javascript in the form of requestAnimationFrame callbacks.

[yhirano, 2016-06-02 13:30:45]

Holding LocalFame sounds natural to me, but you are thinking that it's too
expensive, right?

[Xiaocheng, 2016-06-02 13:45:37]

On 2016/06/02 at 13:30:45, yhirano wrote:
> Holding LocalFame sounds natural to me, but you are thinking that it's too
expensive, right?

That's just my concern. I don't know much about Oilpan, so I'll trust you if you
tell me that the extra cost of holding a Member<LocalFrame> is acceptable.

There's another issue that seems more tricky about the life time of
FrameBlameContext, because by the current design, it's managed outside Blink but
needs to be referenced inside Blink (detailed in #12-#15)

[yhirano, 2016-06-02 13:49:09]

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

[yhirano, 2016-06-02 13:49:10]

+haraken@ as an oilpan expert

[Sami, 2016-06-02 15:15:00]

On 2016/06/02 13:20:46, Xiaocheng wrote:
> The problem is that we must call BlameContext::Leave() at the end, which means
> that the BlameContext must not be deleted in the middle of the attributed
work.
> 
> So it seems that at the beginning, we should not call BlameContext::Enter() on
> the FrameBlameContext itself, but a duplicate of it.

Right, if you held a local frame, when it's time to Leave() you could check that
LocalFrame::client() is still non-null.

[benjhayden, 2016-06-02 19:14:25]

benjhayden@chromium.org changed reviewers:
+ benjhayden@chromium.org

[benjhayden, 2016-06-02 19:14:26]

+Fadi FYI

[benjhayden, 2016-06-02 19:16:08]

benjhayden@chromium.org changed reviewers:
- benjhayden@chromium.org

[benjhayden, 2016-06-02 20:24:16]

benjhayden@chromium.org changed reviewers:
+ benjhayden@chromium.org

[benjhayden, 2016-06-02 20:24:17]

+Ned, Ethan FYI

[benjhayden, 2016-06-02 21:27:14]

benjhayden@chromium.org changed reviewers:
- benjhayden@chromium.org

[tkent, 2016-06-02 21:59:19]

IMO, we should remove WebFrameClient::frameBlameContext, and should have
WebFrameClient::enterBlameContext() and leaveBlameContext() to avoid to handle
BlameContext raw pointers in Blink.

Adding Member<LocalFrame> doesn't add much runtime cost.  It will protect the
LocalFrame object, but won't prevent disconnecting WebFrameClient, I think.

Also, JavaScript code called in V8ScriptRunner::callFunction() can trigger
FrameView::updateStyleAndLayoutIfNeededRecursiveInternal.  Does BlameContext
support such reentrancy? i.e.  Enter() -> Enter() -> Leave() -> Leave().

[haraken, 2016-06-03 00:16:39]

m_frameBlameContext already has a pointer to the Frame, which will keep the
Frame alive, right?

With Oilpan, you shouldn't need a protecting pointer because Oilpan's
conservative GC scans all on-stack pointers.

[Xiaocheng, 2016-06-03 03:22:18]

Thanks for the comments. 

I think I have identified the core problem, so let me put it clear. If we have
Enter()ed a BlameContext, to ensure the correctness of the attribution, we must
have a pairing Leave() on the same BlameContext in the future. Otherwise,
everything after the Enter() will be attributed to the BlameContext.
Unfortunately, we have no way to do so if the BlameContext is deleted when we
need to Leave().

For this patch, a LocalFrame's blame context is deleted when Frame::detach() is
called. And the blame context is not managed by Oilpan, so we have no way to
keep it alive.

Kent: Yes, it supports reentrancy, according to section "Context selection" of
the design doc.

Kentaro: Nope, |m_frameBlameContext| is just a plain collection of some strings
and integers. And it's created outside Blink, so it doesn't affect the removal
of any object managed by Oilpan. Its removal is not affected by holding any
reference from Blink code or keeping any Blink object alive, either.

Sami: What happens if we import the following events in Trace Viewer (ordered by
ascending timestamps)?

1. Object created event of a BlameContext
2. Snapshot event of the BC
3. Context enter event into the BC
4. Object destroyed event of the BC
5. Some arbitrary work
6. Context leave event

Is this trace considered valid? If so, can the work in step 5 be attributed to
the BC, and can the object reference be joined correctly?

If we allow such kind of trace, then there is an easy solution to the problem
that I raised. When a ScopedFrameBlamer is initialized, it doesn't store a ptr
to the BlameContext, but instead, store a duplicate of all the information (cat,
scope, type, name, id) of the BC. Then we can use the duplicate to Enter() and
Leave() without worrying about whether the original BC is deleted or not. I
guess the current memory leak of FrameBlameContext can be fixed in the same way
-- wherever we are storing a ptr to a FrameBlameContext, we store a duplicate
instead.

If we don't allow such kind of trace, then the only solution seems to be
implementing a Blink-side subclass of BlameContext, to which Blink's work is
attributed. In Trace Viewer, we need to build connection among FrameTreeNode,
RenderFrame and this new class to obtain the real cost attributed to a frame. It
perhaps does something good by allowing us to further divide the cost into three
parts: browser, non-Blink renderer and Blink.

[Sami, 2016-06-03 11:33:47]

On 2016/06/03 03:22:18, Xiaocheng wrote:
> Thanks for the comments. 
> 
> I think I have identified the core problem, so let me put it clear. If we have
> Enter()ed a BlameContext, to ensure the correctness of the attribution, we
must
> have a pairing Leave() on the same BlameContext in the future. Otherwise,
> everything after the Enter() will be attributed to the BlameContext.
> Unfortunately, we have no way to do so if the BlameContext is deleted when we
> need to Leave().
> 
> For this patch, a LocalFrame's blame context is deleted when Frame::detach()
is
> called. And the blame context is not managed by Oilpan, so we have no way to
> keep it alive.
> 
> Kent: Yes, it supports reentrancy, according to section "Context selection" of
> the design doc.
> 
> Kentaro: Nope, |m_frameBlameContext| is just a plain collection of some
strings
> and integers. And it's created outside Blink, so it doesn't affect the removal
> of any object managed by Oilpan. Its removal is not affected by holding any
> reference from Blink code or keeping any Blink object alive, either.
> 
> Sami: What happens if we import the following events in Trace Viewer (ordered
by
> ascending timestamps)?
> 
> 1. Object created event of a BlameContext
> 2. Snapshot event of the BC
> 3. Context enter event into the BC
> 4. Object destroyed event of the BC
> 5. Some arbitrary work
> 6. Context leave event
> 
> Is this trace considered valid? If so, can the work in step 5 be attributed to
> the BC, and can the object reference be joined correctly?
> 
> If we allow such kind of trace, then there is an easy solution to the problem
> that I raised. When a ScopedFrameBlamer is initialized, it doesn't store a ptr
> to the BlameContext, but instead, store a duplicate of all the information
(cat,
> scope, type, name, id) of the BC. Then we can use the duplicate to Enter() and
> Leave() without worrying about whether the original BC is deleted or not. I
> guess the current memory leak of FrameBlameContext can be fixed in the same
way
> -- wherever we are storing a ptr to a FrameBlameContext, we store a duplicate
> instead.
> 
> If we don't allow such kind of trace, then the only solution seems to be
> implementing a Blink-side subclass of BlameContext, to which Blink's work is
> attributed. In Trace Viewer, we need to build connection among FrameTreeNode,
> RenderFrame and this new class to obtain the real cost attributed to a frame.
It
> perhaps does something good by allowing us to further divide the cost into
three
> parts: browser, non-Blink renderer and Blink.

It doesn't seem like we can ever have this kind of trace. Namely, step 6 can't
happen because the BlameContext destructor runs in step 4 and we can't call the
Leave() function after that. The work in step 5 should not be attributed to this
blame context anymore. That said, I think there is a bug in the importer that it
doesn't clear out the BlameContext from the set of active contexts when it is
deleted.

If we fix that, is there any issue left here? For ScopedFrameBlamer we could
either keep a reference to the frame or add enter/leaveBlameContext() helpers on
LocalFrame. I don't have a strong preference either way.

Note that the BlameContext object is also passed into the scheduler (i.e., back
outside blink), so wrapping it in a blink-specific object doesn't quite work.

[Xiaocheng, 2016-06-03 15:31:40]

PTAL at Patch 5, which keeps a reference to the LocalFrame, and uses the
BlameContext only when the client is still connected (in which case the
BlameContext must be alive).

I agree that we should let the Trace Viewer's importer ensure that no slice is
attributed to an already deleted BlameContextInstance, instead of doing some
fancy things here. That has to be done in a separate patch, though, and there's
no dependency with this patch.

[Sami, 2016-06-03 15:57:17]

Thanks, lgtm.

[haraken, 2016-06-03 23:24:06]

bindings/ LGTM

https://codereview.chromium.org/2028523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/2028523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:448:
ScopedFrameBlamer frameBlamer(context->isDocument() ?
toDocument(context)->frame() : nullptr);

I think you need to insert the FrameBlamer to runCompiledScript etc as well. You
can do it in a follow-up CL.

[Xiaocheng, 2016-06-04 12:26:36]

Description was changed from

==========
[ALL IN ONE] Attribute Stuff to Frames with FrameBlamer

This patch attributes the cost of the following functions to individual frames:
- FrameView::updateStyleAndLayoutIfNeededRecursiveInternal
- PageAnimator::serviceScriptedAnimations
- V8ScriptRunner::callFunction

The above attribution is built on a new helper class ScopedFrameBlamer, a
wrapper of a LocalFrame's frame blame context, for automatically calling the
blame context's Enter() and Leave() functions when we enter/exit a scope.

Design doc:
https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8AP...

BUG=546021
==========

to

==========
Attribute Stuff to Frames with FrameBlamer

This patch attributes the cost of the following functions to individual frames:
- FrameView::updateStyleAndLayoutIfNeededRecursiveInternal
- PageAnimator::serviceScriptedAnimations
- V8ScriptRunner::callFunction

The above attribution is built on a new helper class ScopedFrameBlamer, a
wrapper of a LocalFrame's frame blame context, for automatically calling the
blame context's Enter() and Leave() functions when we enter/exit a scope.

Design doc:
https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8AP...

BUG=546021
==========

[tkent, 2016-06-05 21:38:47]

lgtm

[Xiaocheng, 2016-06-06 01:49:15]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2016-06-06 01:49:16]

Thanks for all the comments.

I'll commit Patch 6 which has an extra nullptr check of
m_frame->client()->frameBlameContext().

[Xiaocheng, 2016-06-06 01:49:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2028523002/#ps100001
(title: "Bug fix")

[commit-bot: I haz the power, 2016-06-06 01:49:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2028523002/100001

[yhirano, 2016-06-06 04:16:22]

lgtm

[commit-bot: I haz the power, 2016-06-06 06:30:04]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-06 06:31:11]

Description was changed from

==========
Attribute Stuff to Frames with FrameBlamer

This patch attributes the cost of the following functions to individual frames:
- FrameView::updateStyleAndLayoutIfNeededRecursiveInternal
- PageAnimator::serviceScriptedAnimations
- V8ScriptRunner::callFunction

The above attribution is built on a new helper class ScopedFrameBlamer, a
wrapper of a LocalFrame's frame blame context, for automatically calling the
blame context's Enter() and Leave() functions when we enter/exit a scope.

Design doc:
https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8AP...

BUG=546021
==========

to

==========
Attribute Stuff to Frames with FrameBlamer

This patch attributes the cost of the following functions to individual frames:
- FrameView::updateStyleAndLayoutIfNeededRecursiveInternal
- PageAnimator::serviceScriptedAnimations
- V8ScriptRunner::callFunction

The above attribution is built on a new helper class ScopedFrameBlamer, a
wrapper of a LocalFrame's frame blame context, for automatically calling the
blame context's Enter() and Leave() functions when we enter/exit a scope.

Design doc:
https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8AP...

BUG=546021

Committed: https://crrev.com/bb41bc42d96c37688310a8e8afe9d92960d53a25
Cr-Commit-Position: refs/heads/master@{#397984}
==========

[commit-bot: I haz the power, 2016-06-06 06:31:12]

Patchset 6 (id:??) landed as
https://crrev.com/bb41bc42d96c37688310a8e8afe9d92960d53a25
Cr-Commit-Position: refs/heads/master@{#397984}