Bug 153499 - Web Inspector: console.profile should use the new Sampling Profiler
Summary: Web Inspector: console.profile should use the new Sampling Profiler
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 136466 (view as bug list)
Depends on: 158897
Blocks:
  Show dependency treegraph
 
Reported: 2016-01-26 12:26 PST by Joseph Pecoraro
Modified: 2016-06-20 12:13 PDT (History)
14 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (43.03 KB, patch)
2016-06-14 16:02 PDT, Joseph Pecoraro
joepeck: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix (42.80 KB, patch)
2016-06-14 19:57 PDT, Joseph Pecoraro
timothy: review+
Details | Formatted Diff | Diff
[PATCH] For Landing (48.27 KB, patch)
2016-06-16 18:57 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2016-01-26 12:26:46 PST
* SUMMARY
console.profile should use the new Sampling Profiler.

* NOTES
- Currently it uses the LegacyProfiler. It should work with the new Sampling Profiler.
- How that looks is yet to be determined.
  - Should it enable/disable some set of instruments the frontend has told the backend about?
  - Should it just start/stop the ScriptProfiler? which will send profiles to the frontend when done.
  - Should it send sample payload to the frontend on console.profileEnd?
Comment 1 Radar WebKit Bug Importer 2016-01-26 12:27:06 PST
<rdar://problem/24352431>
Comment 2 Joseph Pecoraro 2016-05-31 22:59:33 PDT
I have this working now that we've removed the LegacyProfiler.

It takes advantage of the frontend informing the backend of the enabled Instruments (which was added for auto record on page navigations).

I just need to clean the patches up, test, and justify the policy choices I made.
Comment 3 Joseph Pecoraro 2016-06-14 15:51:37 PDT
*** Bug 136466 has been marked as a duplicate of this bug. ***
Comment 4 Joseph Pecoraro 2016-06-14 16:02:40 PDT
Created attachment 281294 [details]
[PATCH] Proposed Fix

Depends on bug 158762.
Comment 5 Joseph Pecoraro 2016-06-14 19:57:46 PDT
Created attachment 281316 [details]
[PATCH] Proposed Fix
Comment 6 Timothy Hatcher 2016-06-16 14:19:15 PDT
Comment on attachment 281316 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=281316&action=review

Very clean!

> Source/JavaScriptCore/inspector/agents/InspectorScriptProfilerAgent.cpp:223
> +}
> +void InspectorScriptProfilerAgent::programmaticCaptureStopped()

Newline.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:249
> +    // Allow duplicate unnamed profiles. Disallow duplicate named profiles.
> +    if (!title.isEmpty()) {

Sad this needs duplicated.
Comment 7 Joseph Pecoraro 2016-06-16 18:57:14 PDT
Created attachment 281520 [details]
[PATCH] For Landing
Comment 8 WebKit Commit Bot 2016-06-16 19:26:48 PDT
Comment on attachment 281520 [details]
[PATCH] For Landing

Clearing flags on attachment: 281520

Committed r202152: <http://trac.webkit.org/changeset/202152>
Comment 9 Alexey Proskuryakov 2016-06-17 20:26:49 PDT
The new test is very unstable, timing out very frequently: 

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Ftimeline%2FsetInstruments-programmatic-capture.html

Rolling out.
Comment 10 Alexey Proskuryakov 2016-06-17 20:34:22 PDT
Rolled out in https://trac.webkit.org/r202196
Comment 11 Joseph Pecoraro 2016-06-20 12:09:29 PDT
(In reply to comment #9)
> The new test is very unstable, timing out very frequently: 

This appears to be the same issue that affects all inspector tests.

Locally, I ran the test, and the directory 400 times in Release and Debug mode without issue. I suggest treating this test the same [ Pass Timeout ] until we have time to investigate the systemic inspector test issue. I filed bug 158948 for these specific timeouts.
Comment 12 Joseph Pecoraro 2016-06-20 12:13:12 PDT
<https://trac.webkit.org/changeset/202234> with TestExpectations