* 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.
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.
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.
(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.")
Now, sampling profiler is enabled in EFL / GTK!
I think we can move forward with removing the Legacy Profiler now.
Looking at this again.
Created attachment 278912 [details] [PATCH] Proposed Fix
(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/*
Created attachment 278913 [details] [PATCH] Proposed Fix
Created attachment 278917 [details] [PATCH] Proposed Fix - remove some ManualTests as well
Created attachment 278923 [details] [PATCH] Proposed Fix Removes a few more forward declarations and attempts to fix Windows. This could be the one!
Comment on attachment 278923 [details] [PATCH] Proposed Fix r=me
Comment on attachment 278923 [details] [PATCH] Proposed Fix cq- there are 2 more <JavaScriptCore/Profile.h> includes in Windows.
Created attachment 278928 [details] [PATCH] For Landing Give the bots a go on this one and then I'll cq!
👍🏾
Super nice!
Did this land?
Comment on attachment 278928 [details] [PATCH] For Landing Clearing flags on attachment: 278928 Committed r200924: <http://trac.webkit.org/changeset/200924>
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
(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.
Has anyone had a chance to look into the regression, or should we roll out for now?
(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.
(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.
Rolled out in <http://trac.webkit.org/changeset/200973>.
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.
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.
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>
All reviewed patches have been landed. Closing bug.
Reopening, more parts to land (this is multi-part).
(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.
I tried forcing a full build on the bot to see if that changes things.
(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.
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)
Yes, this is a regression. Put into one test I can reproduce with `jsc`. I'll continue to investigate.
Tracking that issue in bug 157968.
(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).
All parts have landed: <http://trac.webkit.org/changeset/201237> <http://trac.webkit.org/changeset/201239> Closing.