Refactor InspectorWebPerfAgent: update lifecycle management to be per Local Frame root; replace hea…

Refactor InspectorWebPerfAgent: update lifecycle management to be per Local Frame root; replace heapset of locations with frames.

BUG=635596
Committed: https://crrev.com/e4a2d76b144acd15b8600162950e973965ababb9
Cr-Commit-Position: refs/heads/master@{#429149}

[panicker, 2016-10-24 23:51:47]

panicker@chromium.org changed reviewers:
+ caseq@chromium.org, pfeldman@chromium.org

[panicker, 2016-10-24 23:51:47]

Hey folks,
I've updated this to be per local-frame root, as this seems easier to reason
about (than static) and is a good incremental step forward.
We can make this per-process in the future, especially after resolution of OOPIF
handling (discussion ongoing).

I will update / add tests, but wanted to get initial feedback first.

[panicker, 2016-10-26 18:48:21]

Friendly morning ping.

[caseq, 2016-10-26 21:19:41]

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:559: if
(!inspectorWebPerfAgent()->isEnabled())
can we perhaps get rid of enable/disable methods and just create the agent when
it's first enabled and nuke it when it's no longer needed?

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:882:
m_inspectorWebPerfAgentObservers(false),
that's a fancy way to initialize an int :-)

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.h:167: InspectorWebPerfAgent*
inspectorWebPerfAgent() {
Do we really need to expose it?

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right):

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:98: for
(Frame* frame = rootFrame; frame; frame = frame->tree().traverseNext()) {
Can we perhaps avoid traversing the entire frame tree to report the tasks?
Perhaps just maintain a list of interested observers?

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/timing/Performance.cpp (right):

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/timing/Performance.cpp:67: if
(m_observingLongTasks && localRoot && localRoot->inspectorWebPerfAgent()) {
We're supposed to always have the agent for a local root, right?

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/timing/Performance.h (right):

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/timing/Performance.h:67: bool
observeringLongTasks();
typo :)

[panicker, 2016-10-27 23:06:53]

(still working on registering listeners vs walking the frame tree)

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:559: if
(!inspectorWebPerfAgent()->isEnabled())
On 2016/10/26 21:19:41, caseq wrote:
> can we perhaps get rid of enable/disable methods and just create the agent
when
> it's first enabled and nuke it when it's no longer needed?

It could be created and destroyed multiple times during the life of the
LocalFrame - is that  desirable?

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/timing/Performance.cpp (right):

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/timing/Performance.cpp:67: if
(m_observingLongTasks && localRoot && localRoot->inspectorWebPerfAgent()) {
On 2016/10/26 21:19:41, caseq wrote:
> We're supposed to always have the agent for a local root, right?

Yeah, currently.
Though your other suggestion was to keep it only when needed (delete when noone
is observering, re-create when observer registers) right?

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/timing/Performance.h (right):

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/timing/Performance.h:67: bool
observeringLongTasks();
On 2016/10/26 21:19:41, caseq wrote:
> typo :)

Oops. Fixed.

[panicker, 2016-10-28 00:09:25]

PTAL

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:882:
m_inspectorWebPerfAgentObservers(false),
On 2016/10/26 21:19:41, caseq wrote:
> that's a fancy way to initialize an int :-)

:) not needed anymore

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.h:167: InspectorWebPerfAgent*
inspectorWebPerfAgent() {
On 2016/10/26 21:19:41, caseq wrote:
> Do we really need to expose it?

Removed

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right):

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:98: for
(Frame* frame = rootFrame; frame; frame = frame->tree().traverseNext()) {
On 2016/10/26 21:19:41, caseq wrote:
> Can we perhaps avoid traversing the entire frame tree to report the tasks?
> Perhaps just maintain a list of interested observers?

Done.

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/timing/Performance.cpp (right):

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/timing/Performance.cpp:67: if
(m_observingLongTasks && localRoot && localRoot->inspectorWebPerfAgent()) {
On 2016/10/27 23:06:53, Shubhie wrote:
> On 2016/10/26 21:19:41, caseq wrote:
> > We're supposed to always have the agent for a local root, right?
> 
> Yeah, currently.
> Though your other suggestion was to keep it only when needed (delete when
noone
> is observering, re-create when observer registers) right?

Done.

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/timing/Performance.h (right):

https://codereview.chromium.org/2449673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/timing/Performance.h:67: bool
observeringLongTasks();
On 2016/10/27 23:06:53, Shubhie wrote:
> On 2016/10/26 21:19:41, caseq wrote:
> > typo :)
> 
> Oops. Fixed.

Sorry, this is actually fine. It's the getter. Should I inline it?

[panicker, 2016-10-31 20:12:44]

PTAL. Updated unittests.

[caseq, 2016-10-31 22:41:55]

lgtm % comments

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

https://codereview.chromium.org/2449673002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.h:279:
HeapHashSet<Member<Performance>> m_webPerformanceObservers;
Can you rather put that within the InspectorWebPerfAgent? It seems to belong
there logically...

https://codereview.chromium.org/2449673002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right):

https://codereview.chromium.org/2449673002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:40:
InspectorWebPerfAgent::InspectorWebPerfAgent(InspectedFrames* inspectedFrames)
I actually like passing root frame pointer more -- I think using InspectedFrames
here is a bit artificial, considering you're only using root() frame anyway and
generally don't need to depend on any inspector stuff so far.

[panicker, 2016-11-01 03:03:33]

PTAL

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

https://codereview.chromium.org/2449673002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.h:279:
HeapHashSet<Member<Performance>> m_webPerformanceObservers;
On 2016/10/31 22:41:54, caseq wrote:
> Can you rather put that within the InspectorWebPerfAgent? It seems to belong
> there logically...

Done.

https://codereview.chromium.org/2449673002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right):

https://codereview.chromium.org/2449673002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:40:
InspectorWebPerfAgent::InspectorWebPerfAgent(InspectedFrames* inspectedFrames)
On 2016/10/31 22:41:55, caseq wrote:
> I actually like passing root frame pointer more -- I think using
InspectedFrames
> here is a bit artificial, considering you're only using root() frame anyway
and
> generally don't need to depend on any inspector stuff so far.

Done.

[panicker, 2016-11-01 17:20:12]

panicker@chromium.org changed reviewers:
+ dgozman@chromium.org

[panicker, 2016-11-01 17:20:12]

Pavel, Dima - could one of you please review for OWNERS?
Thanks!

[dgozman, 2016-11-01 17:48:36]

lgtm

[panicker, 2016-11-01 17:51:36]

panicker@chromium.org changed reviewers:
- pfeldman@chromium.org

[panicker, 2016-11-01 17:51:37]

thanks for the review!
Will submit soon.

[panicker, 2016-11-01 17:51:59]

The CQ bit was checked by panicker@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 17:52:19]

Dry run: 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, 2016-11-01 17:55:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-01 17:55:05]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[panicker, 2016-11-01 20:20:36]

The CQ bit was checked by panicker@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 20:20:57]

Dry run: 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, 2016-11-01 22:44:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-01 22:44:16]

Dry run: This issue passed the CQ dry run.

[panicker, 2016-11-01 23:05:15]

The CQ bit was checked by panicker@chromium.org

[panicker, 2016-11-01 23:05:15]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org,
dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2449673002/#ps140001
(title: "sync and rebase")

[commit-bot: I haz the power, 2016-11-01 23:05:52]

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, 2016-11-01 23:18:00]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-11-01 23:22:12]

Description was changed from

==========
Refactor InspectorWebPerfAgent: update lifecycle management to be per Local
Frame root; replace heapset of locations with frames.

BUG=635596
==========

to

==========
Refactor InspectorWebPerfAgent: update lifecycle management to be per Local
Frame root; replace heapset of locations with frames.

BUG=635596

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

[commit-bot: I haz the power, 2016-11-01 23:22:13]

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

[krasin1, 2016-11-02 20:06:13]

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

[krasin1, 2016-11-02 20:06:14]

https://codereview.chromium.org/2449673002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/Performance.cpp (right):

https://codereview.chromium.org/2449673002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/Performance.cpp:66: LocalFrame* localRoot
= frame()->localFrameRoot();
For the record, this broke UBSan bots:
https://build.chromium.org/p/chromium.fyi/builders/UBSanVptr%20Linux/builds/1452

The reason is that frame() can return NULL on destruction. What is the correct
fix here? Just quit from the destructor early?

[krasin1, 2016-11-02 20:15:46]

On 2016/11/02 20:06:14, krasin1 wrote:
>
https://codereview.chromium.org/2449673002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/timing/Performance.cpp (right):
> 
>
https://codereview.chromium.org/2449673002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/timing/Performance.cpp:66: LocalFrame*
localRoot
> = frame()->localFrameRoot();
> For the record, this broke UBSan bots:
>
https://build.chromium.org/p/chromium.fyi/builders/UBSanVptr%20Linux/builds/1452
> 
> The reason is that frame() can return NULL on destruction. What is the correct
> fix here? Just quit from the destructor early?

I have sent https://codereview.chromium.org/2470183002/ that avoids the call on
nullptr. Please, take a look, if that's the right way to fix things.

[panickercorp, 2016-11-02 20:17:46]

panicker@google.com changed reviewers:
+ panicker@google.com

[panickercorp, 2016-11-02 20:17:47]

https://codereview.chromium.org/2449673002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/Performance.cpp (right):

https://codereview.chromium.org/2449673002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/Performance.cpp:66: LocalFrame* localRoot
= frame()->localFrameRoot();
On 2016/11/02 20:06:14, krasin1 wrote:
> For the record, this broke UBSan bots:
>
https://build.chromium.org/p/chromium.fyi/builders/UBSanVptr%20Linux/builds/1452
> 
> The reason is that frame() can return NULL on destruction. What is the correct
> fix here? Just quit from the destructor early?

Apologies!
Should that have run on presubmit?

I'll prepare a fix and send right away. Thanks!
(yes cannot assume frame is non-null)

[krasin1, 2016-11-02 20:22:00]

On 2016/11/02 20:17:47, panicker wrote:
>
https://codereview.chromium.org/2449673002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/timing/Performance.cpp (right):
> 
>
https://codereview.chromium.org/2449673002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/timing/Performance.cpp:66: LocalFrame*
localRoot
> = frame()->localFrameRoot();
> On 2016/11/02 20:06:14, krasin1 wrote:
> > For the record, this broke UBSan bots:
> >
>
https://build.chromium.org/p/chromium.fyi/builders/UBSanVptr%20Linux/builds/1452
> > 
> > The reason is that frame() can return NULL on destruction. What is the
correct
> > fix here? Just quit from the destructor early?
> 
> Apologies!
No worries.

> Should that have run on presubmit?
I am working to make this a part of the presubmit queue:
https://crbug.com/612594, but as of now it's not. 
> 
> I'll prepare a fix and send right away. Thanks!
> (yes cannot assume frame is non-null)
Alternatively, you can take a look at
https://codereview.chromium.org/2470183002/