Convert performance monitor to the subscription model.

Convert performance monitor to the subscription model.

BUG=662497
Committed: https://crrev.com/b951047a98329d8b1d8be534db6a763e280db291
Cr-Commit-Position: refs/heads/master@{#431719}

[pfeldman, 2016-11-08 21:12:38]

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

[pfeldman, 2016-11-08 21:12:38]

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

[pfeldman, 2016-11-08 22:47:56]

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

[pfeldman, 2016-11-08 22:48:16]

The CQ bit was unchecked by pfeldman@chromium.org

[caseq, 2016-11-09 00:41:12]

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

https://codereview.chromium.org/2484213003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/PerformanceMonitor.h:58: static void
removeSubscriptions(ExecutionContext*, Client*);
unsubscribeClient or something along these lines?

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

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:115: for (size_t i =
0; i < kAfterLast; ++i)
std::fill(std::begin(m_thresholds), std::end(m_thresholds), 0);

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:130:
m_subscriptions.set(violation, clientThresholds);
DCHECK(violation < kAfterLast)?

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:132:
(*clientThresholds).set(client, threshold);
clientThresholds->set(client, threshold)

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:143: double
oldThresholds[kAfterLast];
bool longTaskObserverEnabled = !!m_thresholds[kLongTask] and get rid of the loop
below.

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:169: m_enabled =
false;
m_enabled = std::count(std::begin(m_thresholds), std::end(m_thresholds), 0) <
(std::end(thresholds) - std::begin(thresholds))

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorLogAgent.cpp (right):

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorLogAgent.cpp:186:
m_performanceMonitor->subscribe(parseViolation(settings->get(i)->getName()),
check for error from parseViolation and return error if it's not less than
kAfterLast.

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4381: { "name":
"name", "type": "string", "enum": ["longTask", "longLayout", "blockedEvent"],
"description": "Entry name." },
"description": "Type of violation."?

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4404:
"description": "start violation reporting."
s/start/Start/

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/Performance.cpp (left):

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/Performance.cpp:68: void
Performance::frameDestroyed() {
Shouldn't you unsubscribe when frame is gone?

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/Performance.cpp (right):

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/Performance.cpp:189:
Performance::sanitizedAttribution(contextFrames, frame());
frame() can be null here, right?

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js (right):

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:55: {name:
'longTask', threshold: 50 },
nit: I'd extract a static var for it and call it ...ViolationConfig

[pfeldman, 2016-11-09 01:39:14]

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

https://codereview.chromium.org/2484213003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/PerformanceMonitor.h:58: static void
removeSubscriptions(ExecutionContext*, Client*);
On 2016/11/09 00:41:11, caseq wrote:
> unsubscribeClient or something along these lines?

Done.

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

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:115: for (size_t i =
0; i < kAfterLast; ++i)
On 2016/11/09 00:41:11, caseq wrote:
> std::fill(std::begin(m_thresholds), std::end(m_thresholds), 0);

Done.

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:130:
m_subscriptions.set(violation, clientThresholds);
On 2016/11/09 00:41:11, caseq wrote:
> DCHECK(violation < kAfterLast)?

Done.

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:132:
(*clientThresholds).set(client, threshold);
On 2016/11/09 00:41:11, caseq wrote:
> clientThresholds->set(client, threshold)

Done.

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:143: double
oldThresholds[kAfterLast];
On 2016/11/09 00:41:11, caseq wrote:
> bool longTaskObserverEnabled = !!m_thresholds[kLongTask] and get rid of the
loop
> below.

Done.

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:169: m_enabled =
false;
On 2016/11/09 00:41:11, caseq wrote:
> m_enabled = std::count(std::begin(m_thresholds), std::end(m_thresholds), 0) <
> (std::end(thresholds) - std::begin(thresholds))

Done.

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorLogAgent.cpp (right):

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorLogAgent.cpp:186:
m_performanceMonitor->subscribe(parseViolation(settings->get(i)->getName()),
On 2016/11/09 00:41:11, caseq wrote:
> check for error from parseViolation and return error if it's not less than
> kAfterLast.

Done.

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4381: { "name":
"name", "type": "string", "enum": ["longTask", "longLayout", "blockedEvent"],
"description": "Entry name." },
On 2016/11/09 00:41:11, caseq wrote:
> "description": "Type of violation."?

Done.

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4404:
"description": "start violation reporting."
On 2016/11/09 00:41:11, caseq wrote:
> s/start/Start/

Done.

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/Performance.cpp (left):

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/Performance.cpp:68: void
Performance::frameDestroyed() {
On 2016/11/09 00:41:11, caseq wrote:
> Shouldn't you unsubscribe when frame is gone?

Done.

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/Performance.cpp:68: void
Performance::frameDestroyed() {
On 2016/11/09 00:41:11, caseq wrote:
> Shouldn't you unsubscribe when frame is gone?

Done.

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/Performance.cpp (right):

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/Performance.cpp:189:
Performance::sanitizedAttribution(contextFrames, frame());
On 2016/11/09 00:41:12, caseq wrote:
> frame() can be null here, right?

Not anymore.

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js (right):

https://codereview.chromium.org/2484213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:55: {name:
'longTask', threshold: 50 },
On 2016/11/09 00:41:12, caseq wrote:
> nit: I'd extract a static var for it and call it ...ViolationConfig

Acknowledged.

[pfeldman, 2016-11-09 01:54:32]

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

[commit-bot: I haz the power, 2016-11-09 01:55:06]

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-09 01:58:50]

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

[commit-bot: I haz the power, 2016-11-09 01:58:51]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[caseq, 2016-11-09 02:14:37]

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

https://codereview.chromium.org/2484213003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:90:
std::move(location));
This does not look good in the loop!

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

https://codereview.chromium.org/2484213003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/PerformanceMonitor.h:114:
HeapHashMap<size_t,
use enum type here? or make this an array?

[pfeldman, 2016-11-09 02:29:37]

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

https://codereview.chromium.org/2484213003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:90:
std::move(location));
On 2016/11/09 02:14:37, caseq wrote:
> This does not look good in the loop!

Done.

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

https://codereview.chromium.org/2484213003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/PerformanceMonitor.h:114:
HeapHashMap<size_t,
On 2016/11/09 02:14:37, caseq wrote:
> use enum type here? or make this an array?

Done.

[pfeldman, 2016-11-09 02:32:28]

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

[commit-bot: I haz the power, 2016-11-09 02:33:05]

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

[pfeldman, 2016-11-09 02:36:35]

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

[commit-bot: I haz the power, 2016-11-09 02:37:12]

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

[caseq, 2016-11-09 02:47:48]

lgtm

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

https://codereview.chromium.org/2484213003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:87: if (it.value <
time) {
nit: nuke { }

https://codereview.chromium.org/2484213003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:165:
static_cast<int>(kAfterLast);
you probably don't need the case here now.

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

https://codereview.chromium.org/2484213003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:145: Violation
violation = static_cast<Violation>(it.key);
drop the cast?

https://codereview.chromium.org/2484213003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/PerformanceMonitor.cpp:165:
static_cast<int>(kAfterLast);
do you still need the cast?

https://codereview.chromium.org/2484213003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorLogAgent.cpp (right):

https://codereview.chromium.org/2484213003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorLogAgent.cpp:210:
ConsoleMessage* message = ConsoleMessage::create(
Wire via reportGenericLocation()?

https://codereview.chromium.org/2484213003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorLogAgent.cpp:219:
ConsoleMessage* message = ConsoleMessage::create(
ditto.

[pfeldman, 2016-11-09 02:49:47]

Mostly done :)

[pfeldman, 2016-11-09 02:51:37]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-11-09 02:51:37]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org
Link to the patchset: https://codereview.chromium.org/2484213003/#ps100001
(title: "for landing")

[commit-bot: I haz the power, 2016-11-09 02:51:48]

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-09 03:46:28]

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

[commit-bot: I haz the power, 2016-11-09 03:46:29]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[panicker, 2016-11-09 17:38:54]

LGTM.
Sorry for jumping in late - mostly a question around showing more meaningful
into to devs.

https://codereview.chromium.org/2484213003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorLogAgent.cpp (right):

https://codereview.chromium.org/2484213003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorLogAgent.cpp:210:
String::format("Long running JavaScript task took %ldms.", lround(time));
since this is local dev we could show more info as to why - right ?
Developers would obviously want to know why / who is responsible.
Could enhance as a follow up.

https://codereview.chromium.org/2484213003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorLogAgent.cpp:219:
lround(duration * 1000));
same comment - which JS or source code location?

[pfeldman, 2016-11-11 20:32:52]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-11-11 20:32:53]

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/2484213003/#ps120001
(title: "monitor npcheck")

[commit-bot: I haz the power, 2016-11-11 20:33:28]

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

[pfeldman, 2016-11-11 21:31:32]

https://codereview.chromium.org/2484213003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorLogAgent.cpp (right):

https://codereview.chromium.org/2484213003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorLogAgent.cpp:210:
String::format("Long running JavaScript task took %ldms.", lround(time));
On 2016/11/09 17:38:54, Shubhie wrote:
> since this is local dev we could show more info as to why - right ?
> Developers would obviously want to know why / who is responsible.
> Could enhance as a follow up.

Yes, we have a mock for the new audits panel UI with that.

https://codereview.chromium.org/2484213003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorLogAgent.cpp:219:
lround(duration * 1000));
On 2016/11/09 17:38:54, Shubhie wrote:
> same comment - which JS or source code location?

We'll need to ask them to re-run page under Timeline recording.

[commit-bot: I haz the power, 2016-11-11 21:49:22]

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

[commit-bot: I haz the power, 2016-11-11 21:49:23]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[pfeldman, 2016-11-11 21:59:59]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-11-11 22:00:00]

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/2484213003/#ps140001
(title: "core export")

[commit-bot: I haz the power, 2016-11-11 22:00:30]

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-12 01:14:08]

Committed patchset #8 (id:140001)

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

Description was changed from

==========
Convert performance monitor to the subscription model.

BUG=662497
==========

to

==========
Convert performance monitor to the subscription model.

BUG=662497

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

[commit-bot: I haz the power, 2016-11-12 01:19:04]

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