Bug 153565

Summary: Remove LegacyProfiler
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: JavaScriptCoreAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, ggaren, joepeck, mark.lam, msaboff, rniwa, ryanhaddad, saam, timothy, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 153466    
Bug Blocks:    
Attachments:
Description Flags
[PATCH] Work in Progress
none
[PATCH] Proposed Fix
none
[PATCH] Proposed Fix
none
[PATCH] Proposed Fix - remove some ManualTests as well
none
[PATCH] Proposed Fix
mark.lam: review+, joepeck: commit-queue-
[PATCH] For Landing
none
[PATCH] Part 1 - ManualTests, LayoutTests, WebCore, WebKit none

Joseph Pecoraro
Reported 2016-01-27 15:30:36 PST
* 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.
Attachments
[PATCH] Work in Progress (315.78 KB, patch)
2016-01-27 15:36 PST, Joseph Pecoraro 		no flags
DetailsFormatted DiffDiff
[PATCH] Proposed Fix (329.40 KB, patch)
2016-05-13 19:52 PDT, Joseph Pecoraro 		no flags
DetailsFormatted DiffDiff
[PATCH] Proposed Fix (335.64 KB, patch)
2016-05-13 20:01 PDT, Joseph Pecoraro 		no flags
DetailsFormatted DiffDiff
[PATCH] Proposed Fix - remove some ManualTests as well (338.34 KB, patch)
2016-05-13 20:44 PDT, Joseph Pecoraro 		no flags
DetailsFormatted DiffDiff
[PATCH] Proposed Fix (340.03 KB, patch)
2016-05-13 22:04 PDT, Joseph Pecoraro 		mark.lam: review+
joepeck: commit-queue-
DetailsFormatted DiffDiff
[PATCH] For Landing (340.83 KB, patch)
2016-05-14 00:49 PDT, Joseph Pecoraro 		no flags
DetailsFormatted DiffDiff
[PATCH] Part 1 - ManualTests, LayoutTests, WebCore, WebKit (196.03 KB, patch)
2016-05-20 18:18 PDT, Joseph Pecoraro 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Joseph Pecoraro
Comment 1 2016-01-27 15:36:09 PST
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 2 2016-01-28 10:51:43 PST
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
Comment 3 2016-01-28 10:58:12 PST
(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
Comment 4 2016-02-07 17:38:15 PST
Now, sampling profiler is enabled in EFL / GTK!
Joseph Pecoraro
Comment 5 2016-04-04 16:03:06 PDT
I think we can move forward with removing the Legacy Profiler now.
Joseph Pecoraro
Comment 6 2016-05-13 17:33:31 PDT
Looking at this again.
Joseph Pecoraro
Comment 7 2016-05-13 19:52:36 PDT
Created attachment 278912 [details] [PATCH] Proposed Fix
Joseph Pecoraro
Comment 8 2016-05-13 19:57:00 PDT
(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
Comment 9 2016-05-13 20:01:06 PDT
Created attachment 278913 [details] [PATCH] Proposed Fix
Joseph Pecoraro
Comment 10 2016-05-13 20:44:39 PDT
Created attachment 278917 [details] [PATCH] Proposed Fix - remove some ManualTests as well
Joseph Pecoraro
Comment 11 2016-05-13 22:04:18 PDT
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 12 2016-05-13 22:41:25 PDT
Comment on attachment 278923 [details] [PATCH] Proposed Fix r=me
Joseph Pecoraro
Comment 13 2016-05-13 23:36:20 PDT
Comment on attachment 278923 [details] [PATCH] Proposed Fix cq- there are 2 more <JavaScriptCore/Profile.h> includes in Windows.
Joseph Pecoraro
Comment 14 2016-05-14 00:49:20 PDT
Created attachment 278928 [details] [PATCH] For Landing Give the bots a go on this one and then I'll cq!
Saam Barati
Comment 15 2016-05-14 09:24:06 PDT
👍🏾
Yusuke Suzuki
Comment 16 2016-05-14 09:56:50 PDT
Super nice!
Blaze Burg
Comment 17 2016-05-14 14:02:07 PDT
Did this land?
WebKit Commit Bot
Comment 18 2016-05-14 15:16:22 PDT
Comment on attachment 278928 [details] [PATCH] For Landing Clearing flags on attachment: 278928 Committed r200924: <http://trac.webkit.org/changeset/200924>
Ryosuke Niwa
Comment 19 2016-05-15 00:21:02 PDT
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
Comment 20 2016-05-15 11:14:13 PDT
(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
Comment 21 2016-05-16 12:49:47 PDT
Has anyone had a chance to look into the regression, or should we roll out for now?
Joseph Pecoraro
Comment 22 2016-05-16 13:12:33 PDT
(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
Comment 23 2016-05-16 15:24:09 PDT
(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
Comment 24 2016-05-16 15:30:49 PDT
Rolled out in <http://trac.webkit.org/changeset/200973>.
Joseph Pecoraro
Comment 25 2016-05-20 17:39:23 PDT
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
Comment 26 2016-05-20 18:18:48 PDT
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 27 2016-05-20 18:59:42 PDT
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
Comment 28 2016-05-20 18:59:52 PDT
All reviewed patches have been landed. Closing bug.
Joseph Pecoraro
Comment 29 2016-05-20 20:17:20 PDT
Reopening, more parts to land (this is multi-part).
Joseph Pecoraro
Comment 30 2016-05-20 20:18:46 PDT
(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
Comment 31 2016-05-20 20:53:08 PDT
I tried forcing a full build on the bot to see if that changes things.
Joseph Pecoraro
Comment 32 2016-05-20 21:12:11 PDT
(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
Comment 33 2016-05-20 21:17:04 PDT
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
Comment 34 2016-05-20 21:23:39 PDT
Yes, this is a regression. Put into one test I can reproduce with `jsc`. I'll continue to investigate.
Joseph Pecoraro
Comment 35 2016-05-20 21:32:05 PDT
Tracking that issue in bug 157968.
Joseph Pecoraro
Comment 36 2016-05-20 21:44:24 PDT
(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
Comment 37 2016-05-20 22:23:02 PDT
All parts have landed: <http://trac.webkit.org/changeset/201237> <http://trac.webkit.org/changeset/201239> Closing.
Note You need to log in before you can comment on or make changes to this bug.