Bug 153500 – Web Inspector: InspectorTimelineAgent doesn't need to recompile functions because it now uses the sampling profiler

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Saam Barati]

...

[Radar WebKit Bug Importer]

<rdar://problem/24352458>

[Saam Barati]

Created attachment 269958 [details]
patch

[WebKit Commit Bot]

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.

[Matt Baker]

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.

[Matt Baker]

(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.

[Joseph Pecoraro]

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.

[Saam Barati]

(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

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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.

[WebKit Commit Bot]

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.

[Timothy Hatcher]

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.

[Joseph Pecoraro]

(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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Joseph Pecoraro]

Interestingly this does regress 1 sourceURL related test. I will investigate tomorrow. I wonder if something was depending on Profiling to get that.

[Joseph Pecoraro]

(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.

[Joseph Pecoraro]

Created attachment 270163 [details]
[PATCH] For Landing

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

Comment on attachment 270163 [details]
[PATCH] For Landing

Clearing flags on attachment: 270163

Committed r195799: <http://trac.webkit.org/changeset/195799>

[Michael Catanzaro]

The rollout in r195916 introduced one JSC test failure on the GTK port:

stress/typedarray-forEach.js.ftl-no-cjit-validate-sampling-profiler

[Michael Catanzaro]

(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.

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

That was in a new bug: bug 153721. Looks like the failures stopped, so this was indeed the culprit.

[Joseph Pecoraro]

(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.

[Joseph Pecoraro]

(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.

[Joseph Pecoraro]

Created attachment 270721 [details]
[PATCH] Updated Fix to address Random Test ASSERTIONs

[Joseph Pecoraro]

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.

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alex Christensen]

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.