Bug 153565 – Remove LegacyProfiler

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Joseph Pecoraro]

* SUMMARY
Remove LegacyProfiler.

The Sampling Profiler does not require extra byte codes / recompilation, and is significantly faster. Once all ports enable SAMPLING_PROFILER we should remove the LegacyProfiler.

Bugs to update when old profiler and tests are removed:
<https://webkit.org/b/148819> Enabling the profiler shouldn't prevent us from performing tail call elimination
<https://webkit.org/b/142157> [EFL] Mark two profiler tests to flaky
<https://webkit.org/b/136067> [GTK] Layout Test fast/profiler/dead-time.html and fast/profiler/stop-profiling-after-setTimeout.html are flaky
<https://webkit.org/b/97791> fast/profiler/apply.html fails with LLInt on 32 bit
<https://webkit.org/b/95764> Crashes in fast/profiler layout tests after r127202
<https://webkit.org/b/49801> REGRESSION (r72351): fast/profiler/throw-exception-from-eval.html fails
<https://webkit.org/b/34230> fast/profiler/simple-no-level-change.html crashed on Snow Leopard Release Bot
<https://webkit.org/b/33490> fast/profiler/profile-calls-in-included-file.html crashed on the leopard Commit Bot

Blockers:
<https://webkit.org/b/153466> [GTK] JSC Sampling profiler is not enabled.

[Joseph Pecoraro]

Created attachment 270052 [details]
[PATCH] Work in Progress

This basically removes everything, including a lot of related code. It needs to update bindings tests and there was one sampling-profiler test failing based on sourceURL in an eval, which might have ben related. But I'm going to table it, until other ports get the sampling profiler, which I think would be the real blocker.

That said, other ports aren't getting profiling data in Web Inspector on trunk if they only have the LegacyProfiler.

We should either:

  1. Encourage ports to complete Sampling Profiler
  2. Fallback to LegacyProfiler on ports that don't have the Sampling Profiler

I'm hopeful for (1) given the performance improvements.

[Geoffrey Garen]

Comment on attachment 270052 [details]
[PATCH] Work in Progress

Why does this patch need to remove tests?

It's nice to wait a bit for other ports to adopt the sampling profiler, but we shouldn't wait too long. I don't think we need to keep old tools working forever to satisfy ports that don't move with us.

[Timothy Hatcher]

(In reply to comment #2)
> Comment on attachment 270052 [details]
> [PATCH] Work in Progress
> 
> Why does this patch need to remove tests?
> 
> It's nice to wait a bit for other ports to adopt the sampling profiler, but
> we shouldn't wait too long. I don't think we need to keep old tools working
> forever to satisfy ports that don't move with us.

Joe does not plan to land this yet, he is waiting for other ports to adopt the sampling profiler (from the description "Once all ports enable SAMPLING_PROFILER we should remove the LegacyProfiler.")

[Yusuke Suzuki]

Now, sampling profiler is enabled in EFL / GTK!

[Joseph Pecoraro]

I think we can move forward with removing the Legacy Profiler now.

[Joseph Pecoraro]

Looking at this again.

[Joseph Pecoraro]

Created attachment 278912 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

(In reply to comment #2)
> Comment on attachment 270052 [details]
> [PATCH] Work in Progress
> 
> Why does this patch need to remove tests?

The tests it removes are all LayoutTests/fast/profiler tests which actually inspect the JSC::Profile data created from console.profile/profileEnd and exposed through Internals. Since the JSC::Profile structure and data are being removed, these tests are no longer valid.

Because the Legacy Profiler is replaced by the Sampling Profiler you can consider these tests as replaced by the Sampling Profiler tests:
Source/JavaScriptCore/tests/stress/sampling-profiler*
LayoutTests/inspector/sampling-profiler/*

[Joseph Pecoraro]

Created attachment 278913 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 278917 [details]
[PATCH] Proposed Fix - remove some ManualTests as well

[Joseph Pecoraro]

Created attachment 278923 [details]
[PATCH] Proposed Fix

Removes a few more forward declarations and attempts to fix Windows. This could be the one!

[Mark Lam]

Comment on attachment 278923 [details]
[PATCH] Proposed Fix

r=me

[Joseph Pecoraro]

Comment on attachment 278923 [details]
[PATCH] Proposed Fix

cq- there are 2 more <JavaScriptCore/Profile.h> includes in Windows.

[Joseph Pecoraro]

Created attachment 278928 [details]
[PATCH] For Landing

Give the bots a go on this one and then I'll cq!

[Saam Barati]

👍🏾

[Yusuke Suzuki]

Super nice!

[Blaze Burg]

Did this land?

[WebKit Commit Bot]

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

Clearing flags on attachment: 278928

Committed r200924: <http://trac.webkit.org/changeset/200924>

[Ryosuke Niwa]

It looks like this patch regressed js/regress/string-replace-generic.html:
https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/r200924%20(5245)/results.html

[Saam Barati]

(In reply to comment #19)
> It looks like this patch regressed js/regress/string-replace-generic.html:
> https://build.webkit.org/results/
> Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/r200924%20(5245)/results.html

that's super weird. That test should not rely on anything about the profiler.

[Ryan Haddad]

Has anyone had a chance to look into the regression, or should we roll out for now?

[Joseph Pecoraro]

(In reply to comment #21)
> Has anyone had a chance to look into the regression, or should we roll out
> for now?

I'll take a look. It is passing locally for me in Release.

[Joseph Pecoraro]

(In reply to comment #22)
> (In reply to comment #21)
> > Has anyone had a chance to look into the regression, or should we roll out
> > for now?
> 
> I'll take a look. It is passing locally for me in Release.

I built trunk r200961 and the test passes consistently for me. The Debug bot sometimes passes, but the Release bot seems to always fail. I don't really know how to debug the failure. EWS bots also liked the patch 5 times... so that makes it even weirder.

Since causing this test to fail seems more serious then removing the legacy profiler, I'm going to just roll it out. Maybe I can approach this by removing the Legacy Profiler in smaller pieces.

Its also possible that removing the Legacy Profiler didn't cause the failure but just helped exposes a latent issue; so regardless it should probably be investigated.

[Joseph Pecoraro]

Rolled out in <http://trac.webkit.org/changeset/200973>.

[Joseph Pecoraro]

I'm going to attempt this again in a few phases to see if 

  1. Remove LayoutTests and References in WebCore
    - this will pave the way for new console.profile without conflicts
  2. Remove LegacyProfiler and related classes
    - just change surface code, not code generation
  3. Remove ProfilerOn and any other code generation
    - carefully remove code from the code generation path

Given that this caused a test to fail previously it would likely have only been in step (3). Since it was non-obvious, I want to remove all the unrelated code first which should hopefully reveal what went wrong the first time, or help expose a real issue that was masked.

[Joseph Pecoraro]

Created attachment 279532 [details]
[PATCH] Part 1 - ManualTests, LayoutTests, WebCore, WebKit

This eliminates LegacyProfiler from everywhere outside of JavaScriptCore. The JSC changes are very minor. Next Part will start removing from JSC.

[WebKit Commit Bot]

Comment on attachment 279532 [details]
[PATCH] Part 1 - ManualTests, LayoutTests, WebCore, WebKit

Clearing flags on attachment: 279532

Committed r201237: <http://trac.webkit.org/changeset/201237>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Reopening, more parts to land (this is multi-part).

[Joseph Pecoraro]

(In reply to comment #29)
> Reopening, more parts to land (this is multi-part).

Wow... Just Part 1 caused the test to fail. Thats... eerie.

[Joseph Pecoraro]

I tried forcing a full build on the bot to see if that changes things.

[Joseph Pecoraro]

(In reply to comment #31)
> I tried forcing a full build on the bot to see if that changes things.

It did not help, the test still fails:
<https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK2%20%28Tests%29/builds/6225>

----

But, not all hope is lost. I have a theory!

Previously, Internals::resetToConsistentState was clearing the state from the previous test and doing:

> Source/WebCore/testing/Internals.cpp:-392
> -    page.inspectorController().setLegacyProfilerEnabled(false);

Which runs this:

> Source/WebCore/inspector/InspectorController.cpp:-436
> -void InspectorController::setLegacyProfilerEnabled(bool enable)
> -{
> -    m_legacyProfilerEnabled = enable;
> -
> -    ErrorString unused;
> -    if (enable) {
> -       ...
> -    } else {
> -        m_instrumentingAgents->setPersistentInspectorTimelineAgent(nullptr);
> -        m_scriptDebugServer.recompileAllJSFunctions();
> -        m_timelineAgent->stop(unused);
> -    }
> -}

And ultimately recompileAllJSFunctions:

> void Debugger::recompileAllJSFunctions()
> {
>    m_vm.deleteAllCode();
> }

So, maybe this can explain the issue!

I want to investigate this a bit before a rollout.

[Joseph Pecoraro]

Yes, this seems likely! I am able to reproduce by running two particular tests in a row (worked out from the bots):

$ run-webkit-tests --release js/regress/string-replace-empty.html js/regress/string-replace-generic.html

Found 2 tests; running 2, skipping 0.
Running 1 WebKitTestRunner.     

[2/2] js/regress/string-replace-generic.html failed unexpectedly (text diff)

[Joseph Pecoraro]

Yes, this is a regression. Put into one test I can reproduce with `jsc`. I'll continue to investigate.

[Joseph Pecoraro]

Tracking that issue in bug 157968.

[Joseph Pecoraro]

(In reply to comment #35)
> Tracking that issue in bug 157968.

Given this is a separate, real issue. I'm probably going to follow-thru and land the rest of the original change (already reviewed) and probably mark the test as flakey while it gets investigated (to keep the bots green).

[Joseph Pecoraro]

All parts have landed:
<http://trac.webkit.org/changeset/201237>
<http://trac.webkit.org/changeset/201239>

Closing.