Bug 153500

Summary: Web Inspector: InspectorTimelineAgent doesn't need to recompile functions because it now uses the sampling profiler
Product: WebKit Reporter: Saam Barati <sbarati>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bburg, buildbot, commit-queue, graouts, joepeck, keith_miller, mark.lam, mattbaker, mcatanzaro, msaboff, nvasilyev, rniwa, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 153722    
Bug Blocks:    
Attachments:
Description Flags
patch
none
[PATCH] Proposed Fix
timothy: review+, buildbot: commit-queue-
Archive of layout-test-results from ews104 for mac-yosemite-wk2
none
Archive of layout-test-results from ews117 for mac-yosemite
none
[PATCH] For Landing
none
[PATCH] Updated Fix to address Random Test ASSERTIONs none

Description Saam Barati 2016-01-26 12:27:54 PST
...
Comment 1 Radar WebKit Bug Importer 2016-01-26 12:28:20 PST
<rdar://problem/24352458>
Comment 2 Saam Barati 2016-01-26 17:40:10 PST
Created attachment 269958 [details]
patch
Comment 3 WebKit Commit Bot 2016-01-26 17:42:15 PST
Attachment 269958 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Matt Baker 2016-01-26 17:53:04 PST
Comment on attachment 269958 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=153500

Nit: fix type in bug description: s/profier/profiler

> Source/JavaScriptCore/ChangeLog:8
> +

An explanation of why the sampling profiler doesn't require a call to Debugger::recompileAllJSFunctions at the time the client is set would be helpful.
Comment 5 Matt Baker 2016-01-26 17:53:43 PST
(In reply to comment #4)
> Comment on attachment 269958 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269958&action=review
> 
> > Source/JavaScriptCore/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=153500
> 
> Nit: fix type in bug description: s/profier/profiler

My spelling of typo was itself a typo. Good grief.
Comment 6 Joseph Pecoraro 2016-01-26 18:41:02 PST
Comment on attachment 269958 [details]
patch

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

I will review this in more detail in a bit. I have to think about it!

> Source/WebCore/ChangeLog:9
> +        No new tests (OOPS!).

We normally just remove this line if there are no tests.
Comment 7 Saam Barati 2016-01-27 00:37:46 PST
(In reply to comment #4)
> Comment on attachment 269958 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269958&action=review
> 
> > Source/JavaScriptCore/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=153500
> 
> Nit: fix type in bug description: s/profier/profiler
> 
> > Source/JavaScriptCore/ChangeLog:8
> > +
> 
> An explanation of why the sampling profiler doesn't require a call to
> Debugger::recompileAllJSFunctions at the time the client is set would be
> helpful.

I'll add an explanation, thanks
Comment 8 Joseph Pecoraro 2016-01-27 11:18:21 PST
Comment on attachment 269958 [details]
patch

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

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:86
>      m_instrumentingAgents.setPersistentInspectorTimelineAgent(this);

So, because we are doing this part here, with InspectorController::profilerEnabled, JavaScriptCore will still be compiling in its Profiling bytecodes.

I think we want a more sweeping change that will also remove that overhead.
Comment 9 Joseph Pecoraro 2016-01-27 16:42:37 PST
Created attachment 270062 [details]
[PATCH] Proposed Fix

This does the same thing, but also changes JSDOMWindow::supportsProfiling() to not be true and cause Profiling byte codes to be included when the inspector is open.

It also does a lot of renaming of things dealing with the LegacyProfiler for clarity.
Comment 10 WebKit Commit Bot 2016-01-27 16:45:03 PST
Attachment 270062 [details] did not pass style-queue:


ERROR: LayoutTests/ChangeLog:9:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 61 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Timothy Hatcher 2016-01-27 17:16:14 PST
Comment on attachment 270062 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/inspector/InspectorController.cpp:406
> +bool InspectorController::legacyProfilerEnabled() const

Maybe even more explicit: legacyProfilerEnabledForLayoutTests() or legacyProfilerEnabledForTests().

> Source/WebCore/inspector/InspectorController.cpp:411
> -void InspectorController::setProfilerEnabled(bool enable)
> +void InspectorController::setLegacyProfilerEnabled(bool enable)

Ditto: setLegacyProfilerEnabledForLayoutTests(bool)

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:201
>  void InspectorTimelineAgent::startFromConsole(JSC::ExecState* exec, const String &title)
>  {
> +    // FIXME: <https://webkit.org/b/153499> Web Inspector: console.profile should use the new Sampling Profiler

Does this need to call m_environment.scriptDebugServer().recompileAllJSFunctions() until that is fixed? It seems like console.profile will be neutered otherwise.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:224
>  RefPtr<JSC::Profile> InspectorTimelineAgent::stopFromConsole(JSC::ExecState* exec, const String& title)
>  {
> +    // FIXME: <https://webkit.org/b/153499> Web Inspector: console.profile should use the new Sampling Profiler

Ditto.
Comment 12 Joseph Pecoraro 2016-01-27 17:22:04 PST
(In reply to comment #11)
> Comment on attachment 270062 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270062&action=review
> 
> > Source/WebCore/inspector/InspectorController.cpp:406
> > +bool InspectorController::legacyProfilerEnabled() const
> 
> Maybe even more explicit: legacyProfilerEnabledForLayoutTests() or
> legacyProfilerEnabledForTests().
> 
> > Source/WebCore/inspector/InspectorController.cpp:411
> > -void InspectorController::setProfilerEnabled(bool enable)
> > +void InspectorController::setLegacyProfilerEnabled(bool enable)
> 
> Ditto: setLegacyProfilerEnabledForLayoutTests(bool)

I have a patch out to remove legacy profiler, but I want to wait to hear back from GTK/EFL/Linux Ports about their plans regarding the sampling profiler. In the off chance that they don't have immediate plans to enable it, we might actually want this for them, non-LayoutTest code.

> 
> > Source/WebCore/inspector/InspectorTimelineAgent.cpp:201
> >  void InspectorTimelineAgent::startFromConsole(JSC::ExecState* exec, const String &title)
> >  {
> > +    // FIXME: <https://webkit.org/b/153499> Web Inspector: console.profile should use the new Sampling Profiler
> 
> Does this need to call
> m_environment.scriptDebugServer().recompileAllJSFunctions() until that is
> fixed? It seems like console.profile will be neutered otherwise.

Correct, this neuters console.profile.

It won't work without the LegacyProfiler enabled, which we don't enable at all outside of tests. We couldn't just recompile, we'd have to "inspectorController.setLegacyProfilerEnabled(true)" to trick JSDOMWindowBase::supportsProfiling into enabling the legacy profiler.

But still I don't think this will even work for the currently executing JavaScript so I didn't even try. I plan on migrating it over to the sampling profiler soon, probably sending StackTraces along with the console.profile Console message. This can now work in JSContext inspection as well now. But I didn't want to do that now.
Comment 13 Build Bot 2016-01-27 17:47:05 PST
Comment on attachment 270062 [details]
[PATCH] Proposed Fix

Attachment 270062 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/747700

New failing tests:
inspector/sampling-profiler/eval-source-url.html
Comment 14 Build Bot 2016-01-27 17:47:09 PST
Created attachment 270068 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 15 Build Bot 2016-01-27 17:48:09 PST
Comment on attachment 270062 [details]
[PATCH] Proposed Fix

Attachment 270062 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/747693

New failing tests:
inspector/sampling-profiler/eval-source-url.html
Comment 16 Build Bot 2016-01-27 17:48:12 PST
Created attachment 270070 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 17 Joseph Pecoraro 2016-01-27 23:08:59 PST
Interestingly this does regress 1 sourceURL related test. I will investigate tomorrow. I wonder if something was depending on Profiling to get that.
Comment 18 Joseph Pecoraro 2016-01-28 17:00:11 PST
(In reply to comment #17)
> Interestingly this does regress 1 sourceURL related test. I will investigate
> tomorrow. I wonder if something was depending on Profiling to get that.

Heh, so it appears to be tail call related:

    // FIXME: We should be able to have tail call elimination with the profiler
    // enabled. This is currently not possible because the profiler expects
    // op_will_call / op_did_call pairs before and after a call, which are not
    // compatible with tail calls (we have no way of emitting op_did_call).
    // https://bugs.webkit.org/show_bug.cgi?id=148819
    , m_inTailPosition(Options::useTailCalls() && !isConstructor() && constructorKind() == ConstructorKind::None && isStrictMode() && !m_shouldEmitProfileHooks)

If I disable this (which used to happen when m_shouldEmitProfileHooks was true with profiling bytecodes) the test passes again.

So I'll see if I can tweak the test to not be affected by tail call optimizations.
Comment 19 Joseph Pecoraro 2016-01-28 17:09:56 PST
Created attachment 270163 [details]
[PATCH] For Landing
Comment 20 WebKit Commit Bot 2016-01-28 17:11:30 PST
Attachment 270163 [details] did not pass style-queue:


ERROR: LayoutTests/ChangeLog:9:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: LayoutTests/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 2 in 62 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 WebKit Commit Bot 2016-01-28 19:46:24 PST
Comment on attachment 270163 [details]
[PATCH] For Landing

Clearing flags on attachment: 270163

Committed r195799: <http://trac.webkit.org/changeset/195799>
Comment 22 Michael Catanzaro 2016-01-30 17:43:25 PST
The rollout in r195916 introduced one JSC test failure on the GTK port:

stress/typedarray-forEach.js.ftl-no-cjit-validate-sampling-profiler
Comment 23 Michael Catanzaro 2016-01-30 19:43:54 PST
(In reply to comment #22)
> The rollout in r195916 introduced one JSC test failure on the GTK port:
> 
> stress/typedarray-forEach.js.ftl-no-cjit-validate-sampling-profiler

All good now that r195911 has been rolled out as well.
Comment 24 Alexey Proskuryakov 2016-01-30 22:56:01 PST
I don't see where I posted links, I must have added my comment to a wrong bug. Here is one - but the crashes were happening on seemingly random tests, so I don't know how reproducible this will be locally.

https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/r195905%20(2570)/results.html
Comment 25 Alexey Proskuryakov 2016-01-30 22:58:51 PST
That was in a new bug: bug 153721. Looks like the failures stopped, so this was indeed the culprit.
Comment 26 Joseph Pecoraro 2016-02-04 20:06:20 PST
(In reply to comment #25)
> That was in a new bug: bug 153721. Looks like the failures stopped, so this
> was indeed the culprit.

I think this could happen if a test had a console.profile() without a matching console.profileEnd(). So if this bot ran a fast/profiler test with such conditions, the InspectorTimelineAgent would have been left running. I should be able to test this theory. Ultimately this stuff will probably be getting ripped out soon anyways and console.profile will change behavior.
Comment 27 Joseph Pecoraro 2016-02-04 20:31:26 PST
(In reply to comment #26)
> (In reply to comment #25)
> > That was in a new bug: bug 153721. Looks like the failures stopped, so this
> > was indeed the culprit.
> 
> I think this could happen if a test had a console.profile() without a
> matching console.profileEnd(). So if this bot ran a fast/profiler test with
> such conditions, the InspectorTimelineAgent would have been left running. I
> should be able to test this theory. Ultimately this stuff will probably be
> getting ripped out soon anyways and console.profile will change behavior.

Basically I added back TimelineAgent->start/stop in  InspectorController::setLegacyProfilerEnabled. It doesn't affect real page inspection, but basically just test code. Windows does apparently call down into this, so this would help that case too.
Comment 28 Joseph Pecoraro 2016-02-04 20:33:52 PST
Created attachment 270721 [details]
[PATCH] Updated Fix to address Random Test ASSERTIONs
Comment 29 Joseph Pecoraro 2016-02-04 20:34:35 PST
Comment on attachment 270721 [details]
[PATCH] Updated Fix to address Random Test ASSERTIONs

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

> Source/WebCore/inspector/InspectorController.cpp:430
>      if (enable) {
>          m_instrumentingAgents->setPersistentInspectorTimelineAgent(m_timelineAgent);
> +        m_scriptDebugServer.recompileAllJSFunctions();
>          m_timelineAgent->start(unused);
>      } else {
>          m_instrumentingAgents->setPersistentInspectorTimelineAgent(nullptr);
> +        m_scriptDebugServer.recompileAllJSFunctions();
>          m_timelineAgent->stop(unused);
>      }
>  }

This is the only change from the last patch. I kept in the start/stop calls here, since apparently later tests were running with the timeline agent still enabled.
Comment 30 WebKit Commit Bot 2016-02-04 20:36:17 PST
Attachment 270721 [details] did not pass style-queue:


ERROR: LayoutTests/ChangeLog:9:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: LayoutTests/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 2 in 64 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 31 WebKit Commit Bot 2016-02-04 21:24:48 PST
Comment on attachment 270721 [details]
[PATCH] Updated Fix to address Random Test ASSERTIONs

Clearing flags on attachment: 270721

Committed r196165: <http://trac.webkit.org/changeset/196165>
Comment 32 WebKit Commit Bot 2016-02-04 21:24:54 PST
All reviewed patches have been landed.  Closing bug.
Comment 33 Alex Christensen 2016-02-11 11:13:47 PST
Here are all the commits associated with this bug:
http://trac.webkit.org/changeset/195799 - original commit
http://trac.webkit.org/changeset/195828 - Windows build fix
http://trac.webkit.org/changeset/195916 - rolled out r195828 and r195799
http://trac.webkit.org/changeset/196165 - re-committed with everything fixed.