PerformanceMonitor: do not maintain a vector of frames in the monitor, single ExecutionContext memb…

PerformanceMonitor: do not maintain a vector of frames in the monitor, single ExecutionContext member should suffice.

Committed: https://crrev.com/f8a7ef85fb385739446dc3178c6b84fadd84ae5f
Cr-Commit-Position: refs/heads/master@{#436128}

[pfeldman, 2016-12-01 02:02:25]

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

[caseq, 2016-12-01 02:04:04]

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

https://codereview.chromium.org/2539323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:428:
m_performanceMonitor->shutdown();
This does not look good already.

[pfeldman, 2016-12-01 02:17:54]

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

[caseq, 2016-12-01 02:32:46]

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

https://codereview.chromium.org/2539323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/timing/Performance.cpp:172: if (!taskContext ||
!taskContext->isDocument() ||
This will result in always returning kUnknownAttribution instead of
kAmbiguousAttribution (can you fix the typo as a drive by btw?)

[pfeldman, 2016-12-01 02:42:11]

all done, ptal!

[caseq, 2016-12-01 02:45:30]

https://codereview.chromium.org/2539323002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2539323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:394: if (!tree().parent() ||
!tree().parent()->isLocalFrame())
Why not just check for isLocalRoot()?

[caseq, 2016-12-01 02:48:54]

https://codereview.chromium.org/2539323002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp (right):

https://codereview.chromium.org/2539323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:205:
m_subscriptions.clear();
Should you also clear thresholds here?

[pfeldman, 2016-12-01 02:50:48]

all done.

[caseq, 2016-12-01 02:51:56]

lgtm, but as discussed please also get one from Shubhie and make sure there's no
perf regressions.

[panicker, 2016-12-01 02:58:17]

On 2016/12/01 02:50:48, pfeldman wrote:
> all done.

Hey folks, Can we talk about this in the morning?
I'd like to change this soon so that even if there are multiple contexts
involved, I want to report attribution if "my frame" is one of them.
So this CL may need some changes.

[pfeldman, 2016-12-02 01:08:30]

On 2016/12/01 02:58:17, Shubhie wrote:
> On 2016/12/01 02:50:48, pfeldman wrote:
> > all done.
> 
> Hey folks, Can we talk about this in the morning?
> I'd like to change this soon so that even if there are multiple contexts
> involved, I want to report attribution if "my frame" is one of them.
> So this CL may need some changes.

So, should I land this?

[panicker, 2016-12-02 01:29:58]

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

https://codereview.chromium.org/2539323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/PerformanceMonitor.h:150:
Member<ExecutionContext> m_taskExecutionContext;
This should still be a Vector or Set.
Other than that I don't have any issues with the CL.
Although the CL description suggests that this is the intended change, could you
update CL description?

[pfeldman, 2016-12-02 01:37:14]

> This should still be a Vector or Set.
> Other than that I don't have any issues with the CL.
> Although the CL description suggests that this is the intended change, could
you
> update CL description?

Started rolling it back and I am thinking that I should not be. This change
seems to reduce the amount of work that we do while instrumenting and does not
regress any of the functionality. I'd suggest landing it since it is incremental
improvement - we don't need set or vector for tasks. When you introduce your
vector, it'll be a vector of scripts rather that the tasks.

[panicker, 2016-12-02 02:38:15]

On 2016/12/02 01:37:14, pfeldman wrote:
> > This should still be a Vector or Set.
> > Other than that I don't have any issues with the CL.
> > Although the CL description suggests that this is the intended change, could
> you
> > update CL description?
> 
> Started rolling it back and I am thinking that I should not be. This change
> seems to reduce the amount of work that we do while instrumenting and does not
> regress any of the functionality. I'd suggest landing it since it is
incremental
> improvement - we don't need set or vector for tasks. When you introduce your
> vector, it'll be a vector of scripts rather that the tasks.

I think it depends on whether frame contexts from different origins
(cross-origin) can be involved within a task, which I believe is possible even
though unlikely.
In that case we cannot avoid keeping the Frame contexts around, because it would
be major security breach to deliver a scripts from Origin Foo to Origin Bar.
Does that make sense? I think we can verify the above assumption and make a
decision based on that?

[pfeldman1, 2016-12-02 02:55:52]

I don't think this relates to tasks vs contexts. This change does not regress
existing behavior / reporting + you'll be collecting script/context pairs

[panicker, 2016-12-02 16:30:29]

On 2016/12/02 02:55:52, pfeldman1 wrote:
> I don't think this relates to tasks vs contexts. This change does not regress
> existing behavior / reporting + you'll be collecting script/context pairs

Yes I will change the HashSet to Vector<pair<Frame, String>> and drop the
boolean, so it seems unnecessary to change from HashSet<Frame> to pointer +
boolean.
But if you want to avoid updating the CL go ahead, and I'll send a follow up CL
to change it again.

[pfeldman1, 2016-12-02 17:18:38]

That's the thing, as we discussed, I don't think it'll be <Frame, String> pair,
it'll rather be <ExecutionContext, Weak<Function>> or Script pair. Otherwise you
are putting unnecessary load to the instrumentation. In fact, even constructing
Weak needs assessment. And that's why I think it is worth landing as is - it'll
take you/us time to figure out efficient instrumentation. Unless we give up and
trigger the "slow mode".

[panicker, 2016-12-02 17:20:15]

On 2016/12/02 17:18:38, pfeldman1 wrote:
> That's the thing, as we discussed, I don't think it'll be <Frame, String>
pair,
> it'll rather be <ExecutionContext, Weak<Function>> or Script pair. Otherwise
you
> are putting unnecessary load to the instrumentation. In fact, even
constructing
> Weak needs assessment. And that's why I think it is worth landing as is -
it'll
> take you/us time to figure out efficient instrumentation. Unless we give up
and
> trigger the "slow mode".

Ok SGTM.

[panicker, 2016-12-02 17:20:21]

lgtm

[pfeldman, 2016-12-02 21:25:09]

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

[commit-bot: I haz the power, 2016-12-02 21:25:52]

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-12-02 23:04:00]

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

[commit-bot: I haz the power, 2016-12-02 23:04:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pfeldman, 2016-12-02 23:56:28]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-12-02 23:56:28]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org,
panicker@chromium.org
Link to the patchset: https://codereview.chromium.org/2539323002/#ps80001
(title: "off-by-one in depth fixed")

[commit-bot: I haz the power, 2016-12-02 23:57:08]

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-12-03 01:30:48]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480722988456960,
"parent_rev": "be59c8c76cdcc300fba910faac6da665972cabe2", "commit_rev":
"b356593b64b632c2b684da2c9c31c1f6f93364ba"}

[commit-bot: I haz the power, 2016-12-03 01:31:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-03 01:34:00]

Description was changed from

==========
PerformanceMonitor: do not maintain a vector of frames in the monitor, single
ExecutionContext member should suffice.
==========

to

==========
PerformanceMonitor: do not maintain a vector of frames in the monitor, single
ExecutionContext member should suffice.

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

[commit-bot: I haz the power, 2016-12-03 01:34:01]

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