...
<rdar://problem/24352458>
Created attachment 269958 [details] patch
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 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.
(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 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.
(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 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.
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.
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 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.
(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 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
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 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
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
Interestingly this does regress 1 sourceURL related test. I will investigate tomorrow. I wonder if something was depending on Profiling to get that.
(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.
Created attachment 270163 [details] [PATCH] For Landing
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 on attachment 270163 [details] [PATCH] For Landing Clearing flags on attachment: 270163 Committed r195799: <http://trac.webkit.org/changeset/195799>
The rollout in r195916 introduced one JSC test failure on the GTK port: stress/typedarray-forEach.js.ftl-no-cjit-validate-sampling-profiler
(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.
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
That was in a new bug: bug 153721. Looks like the failures stopped, so this was indeed the culprit.
(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.
(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.
Created attachment 270721 [details] [PATCH] Updated Fix to address Random Test ASSERTIONs
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.
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 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>
All reviewed patches have been landed. Closing bug.
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.