...
<rdar://problem/24066360>
Created attachment 268339 [details] WIP This is the work I've done while doing the sampling profiler.
Comment on attachment 268339 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=268339&action=review Looking great so far! I added some comments mostly on the Web Inspector side. I'll need a Changelog to make full sense of the JSC parts :) > Source/JavaScriptCore/runtime/SamplingProfiler.cpp:452 > + return "<host>"; Don't we have some way to get the native function's name? > Source/JavaScriptCore/runtime/SamplingProfiler.cpp:498 > + if (stackFrame.frameType == FrameType::Host) Does this mean we will combine all native stack frames in the CCT? This seems wrong. > Source/JavaScriptCore/runtime/SamplingProfiler.cpp:567 > + comma(); Neat. > Source/JavaScriptCore/runtime/SamplingProfiler.h:79 > + JS_EXPORT_PRIVATE void noticeCurrentThreadAsJSCExecutionThread(); This doesn't seem to be used? > Source/JavaScriptCore/runtime/SamplingProfiler.h:93 > + double m_totalTime; Does JSC code use member initializers in the header? We have started doing this for inspector code. > Source/WebCore/inspector/InspectorTimelineAgent.cpp:47 > +#include <runtime/SamplingProfiler.h> This is fine for now in WebCore TimelineAgent, but before landing you'll want to move it to Joe's new domain in JSC, which is JSContext-compatible. > Source/WebCore/inspector/InspectorTimelineAgent.cpp:91 > m_environment.scriptDebugServer().recompileAllJSFunctions(); Could we move the recompilation call inside ensureSamplingProfiler()? It seems error-prone to rely on the caller doing it. Or if this is just supporting the legacy profiler, I guess we'll remove it later. > Source/WebCore/inspector/InspectorTimelineAgent.cpp:124 > + out_stackTraces = samplingProfiler->stackTracesForInspector(); I am leaning towards wanting stackTracesForInspector stuff out of the SamplingProfiler, but it looks like we need to hold the lock anyway when generating. > Source/WebCore/inspector/InspectorTimelineAgent.cpp:126 > + } Should we combine this into a takeStackTracesForInspector()? When would we get the data but not clear the buffer? > Source/WebCore/inspector/InspectorTimelineAgent.h:46 > +class CallingContextTree; What's it for? > Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:251 > + let startTime = 0; Not sure what this does, assume it's generating some data automatically. > Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:548 > + return; ? > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:50 > + let timeStamp = stackTrace.timeStamp; In the inspector, I think we have been using timestamp/Timestamp rather than the camel cased version. > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:57 > + for (let i = stackFrames.length; i--; ) { whoa, that was an unexpected condition. > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:76 > +let __uid = 0; :) Put it on one of the constructors. > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:84 > + this._sourceID = sourceID; Probably want to add a sourceLocation getter (returning a cached or LazySourceLocation) eventually so we can use this in the UI. > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:86 > + this._column = column; How are native stack frames represented at this level? Do they have line:column? Maybe we should propagate frame type etc. to this level too. > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:115 > + forEachChild(f) Should this return the value of the callback? Then above we can do: roots = this.forEachChild(...) > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:117 > + for (let pname of Object.getOwnPropertyNames(this._children)) does this have a stable iteration order? It would be great to avoid extra sorting in the view classes. > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:121 > + toCPUProfileNode(tree) Eventually we might be able to use CCT directly in the View classes, or do the conversion there. What do you think? We still need to handle CPUProfileNode for remote inspection of older versions. > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:138 > + let endTime = Math.max(...this._timeStamps); Cool! > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:140 > + callInfo.callCount = this._timeStamps.length; // Totally not true, but oh well, lets show something in the UI. If nothing is conditionally assigned, you can use an object literal here. > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:151 > + return stackFrame.sourceID + ":" + stackFrame.line + ":" + stackFrame.column; Is JSC smart enough to memoize this?
Comment on attachment 268339 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=268339&action=review >> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:452 >> + return "<host>"; > > Don't we have some way to get the native function's name? Probably but I haven't looked into if it's okay to execute that code while JSC is paused. We might need to do something like that lazily when generating stack traces for the inspector. >> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:498 >> + if (stackFrame.frameType == FrameType::Host) > > Does this mean we will combine all native stack frames in the CCT? This seems wrong. currently, yes. We need a way to have the name be a distinguishing factor or some other form of information. >> Source/JavaScriptCore/runtime/SamplingProfiler.h:79 >> + JS_EXPORT_PRIVATE void noticeCurrentThreadAsJSCExecutionThread(); > > This doesn't seem to be used? I dumped this file from my main sampling profiler patch. It's used there. >> Source/JavaScriptCore/runtime/SamplingProfiler.h:93 >> + double m_totalTime; > > Does JSC code use member initializers in the header? We have started doing this for inspector code. Yeah we sometimes do. I'm not sure what the JSC's de-facto style is. I've seen it both ways. >> Source/WebCore/inspector/InspectorTimelineAgent.cpp:47 >> +#include <runtime/SamplingProfiler.h> > > This is fine for now in WebCore TimelineAgent, but before landing you'll want to move it to Joe's new domain in JSC, which is JSContext-compatible. Yeah, I'm waiting on Joe's patch before I make this patch really ready for landing. >> Source/WebCore/inspector/InspectorTimelineAgent.cpp:91 >> m_environment.scriptDebugServer().recompileAllJSFunctions(); > > Could we move the recompilation call inside ensureSamplingProfiler()? It seems error-prone to rely on the caller doing it. Or if this is just supporting the legacy profiler, I guess we'll remove it later. Yeah I'm not sure the reason for this. We definitely don't need it for the sampling profiler. If it's for the legacy profiler I'll remove it when I start working on this patch. >> Source/WebCore/inspector/InspectorTimelineAgent.cpp:126 >> + } > > Should we combine this into a takeStackTracesForInspector()? When would we get the data but not clear the buffer? Never ATM. We could combine them. It's just a matter of style. Maybe we would want to not clear it in the future, but it seems unlikely. >> Source/WebCore/inspector/InspectorTimelineAgent.h:46 >> +class CallingContextTree; > > What's it for? Old code that was removed. >> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:548 >> + return; > > ? While I was testing the patch, I only wanted my records to show up in the UI. I'll remove this when I put this patch into shape for landing. >> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:57 >> + for (let i = stackFrames.length; i--; ) { > > whoa, that was an unexpected condition. :) >> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:84 >> + this._sourceID = sourceID; > > Probably want to add a sourceLocation getter (returning a cached or LazySourceLocation) eventually so we can use this in the UI. SGTM >> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:86 >> + this._column = column; > > How are native stack frames represented at this level? Do they have line:column? Maybe we should propagate frame type etc. to this level too. Yeah propagating more information will be useful. I also think it would be cool to display the compiler tier in the future. >> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:115 >> + forEachChild(f) > > Should this return the value of the callback? Then above we can do: roots = this.forEachChild(...) Like create an array of all the results? I suppose it could, or we could build a function on top of this one that does that? maybe: "mapChildren(f)" > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:137 > + let startTime = Math.min(...this._timeStamps); // Time is in microseconds. Convert to seconds. this comment is out of date. >> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:140 >> + callInfo.callCount = this._timeStamps.length; // Totally not true, but oh well, lets show something in the UI. > > If nothing is conditionally assigned, you can use an object literal here. SGTM >> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:151 >> + return stackFrame.sourceID + ":" + stackFrame.line + ":" + stackFrame.column; > > Is JSC smart enough to memoize this? not sure.
Comment on attachment 268339 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=268339&action=review >> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:117 >> + for (let pname of Object.getOwnPropertyNames(this._children)) > > does this have a stable iteration order? It would be great to avoid extra sorting in the view classes. Not sure. I tend to think yes but I can verify. >> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:121 >> + toCPUProfileNode(tree) > > Eventually we might be able to use CCT directly in the View classes, or do the conversion there. What do you think? We still need to handle CPUProfileNode for remote inspection of older versions. That seems reasonable to me. We can avoid doing the unnecessary conversion. I want to do some clean up of this code a lot before landing. A lot of this was written really quickly just to get a UI showing while doing local testing.
Created attachment 268831 [details] WIP Almost done. Just want to put up a WIP for people to take a look. I still need to address some of Brian's original questions/suggestions before I put this up for review.
Comment on attachment 268831 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=268831&action=review > Source/JavaScriptCore/inspector/protocol/ScriptProfiler.json:27 > + { "name": "line", "type": "integer", "description": "Line number." }, > + { "name": "column", "type": "integer", "description": "Column" }, > + { "name": "url", "type": "string", "description": "url of js." } You can drop descriptions that don't add anything. This doesn't appear to be optional, what if there is no URL? > Source/JavaScriptCore/inspector/protocol/ScriptProfiler.json:34 > + { "name": "timeStamp", "type": "number", "description": "Time since the start of recording in microseconds." }, We have been preferring "timestamp" (all lowercase). > Source/JavaScriptCore/inspector/protocol/ScriptProfiler.json:35 > + { "name": "stackFrames", "type": "array", "items": { "$ref": "StackFrame" }, "description": "List of stack frames." } Maybe this description could say "First frame is the bottom of the stack and last frame is the top of the stack." > Source/JavaScriptCore/inspector/protocol/ScriptProfiler.json:43 > + "id": "StackTraces", > + "type": "object", > + "properties": [ > + { "name": "totalTime", "type": "number", "description": "Total time spent profiling in microseconds." }, > + { "name": "stackTraces", "type": "array", "items": { "$ref": "StackTrace", "description": "Array of stack traces." } } I wonder if we should name this a "Samples". > Source/JavaScriptCore/inspector/protocol/ScriptProfiler.json:44 > + ] We should change startTracking's "profile" parameter to be something like "includeSamples". > Source/JavaScriptCore/inspector/protocol/ScriptProfiler.json:79 > + { "name": "stackTraces", "$ref": "StackTraces", "description": "Stack traces." } Likewise here, "samples"? Later, if we add more information to the samples it would be in the Samples object, even if it has nothing to do with "Stack traces". > Source/JavaScriptCore/runtime/SamplingProfiler.cpp:565 > + if (frameType == FrameType::Unknown) > + return "<unknown-url>"; Oh, I see, it'll send an unknown string. Not sure how I feel about these weird strings. > Source/JavaScriptCore/runtime/SamplingProfiler.h:107 > void processUnverifiedStackTraces(); // You should call this only after acquiring the lock. This could follow the convention of the other methods and be passed the LockHolder& to prove you have the lock. > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:32 > + this._root = new WebInspector.CCTNode(-1, -1, -1, "<root>", "I.am.not.a.real.url"); Heh > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:41 > + get root() > + { > + return this._root; > + } > + > + get totalNumSamples() { return this._totalNumSamples; } FWIW: I really like this simple style for basic getters/setters. We could update a lot of our classes to this and save a LOT of lines. > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:133 > + addTimeStamp(t) > + { > + this._timeStamps.push(t); > + } > + > + forEachChild(f) Style: we tend not to abbreviate, for clarity. t => timestamp, f => callback. > Source/WebInspectorUI/UserInterface/Views/ScriptTimelineView.js:42 > + //columns.callCount.title = WebInspector.UIString("Calls"); > + columns.callCount.title = WebInspector.UIString("Samples"); // FIXME: do this conditionally based on sampling profiler being enabled. Yeah, what you can do here is create the "columns" object dynamically, so that the order is what you expect. Something like: var columns = {}; columns.location = {}; columns.location.title = WebInspector.UIString("Location"); columns.location.width = "15%"; if (window.ScriptProfilerAgent) { columns.samples = {}; columns.samples.title = ...; ...; } else { // COMPATIBILITY(iOS 9): ScriptProfilerAgent did not exist yet, we had call counts, not samples. columns.callCount = {}; columns.callCount.title = ...; ...; } Normally we might create a "LegacyScriptTimelineView.js" for the old cases, but here I think this might be the only difference so we could share.
Comment on attachment 268831 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=268831&action=review >> Source/JavaScriptCore/runtime/SamplingProfiler.h:107 >> void processUnverifiedStackTraces(); // You should call this only after acquiring the lock. > > This could follow the convention of the other methods and be passed the LockHolder& to prove you have the lock. I wish, but the GC does some subtle locking around calling this function that doesn't really lend itself well to this pattern, unfortunately.
Created attachment 268905 [details] WIP almost done.
Comment on attachment 268905 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=268905&action=review > Source/WebInspectorUI/UserInterface/Views/ScriptTimelineView.js:37 > + var columns = {location: {}, [isSamplingProfiler ? "samples" : "callCount"]: {}, startTime: {}, totalTime: {}, selfTime: {}, averageTime: {}}; Pro. But this is going to mean in places where we currently have "callCount" (ScriptTimelineDataGridNode for example) we will want to also have `samples`. You will discover that eventually.
Created attachment 268991 [details] patch
Created attachment 268992 [details] patch changed one line.
Comment on attachment 268992 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=268992&action=review > Source/JavaScriptCore/inspector/agents/InspectorScriptProfilerAgent.cpp:68 > + if (includeSamples && *includeSamples) { all sampling profiler related things in this file need to be behind ENABLE(SAMPLING_PROFILER). I've changed this locally. > Source/JavaScriptCore/inspector/protocol/ScriptProfiler.json:24 > + { "name": "functionName", "type": "string", "description": "Name of the function in stack frame or <global>." }, I'm renaming this to "name" > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:189 > + return stackFrame.sourceID + ":" + stackFrame.line + ":" + stackFrame.column; This needs to also hash on function name.
Created attachment 268996 [details] patch fix build for !ENABLE(SAMPLING_PROFILER) and rename "functionName" to "name"
Created attachment 268997 [details] patch changed "description" for some protocol fields to be true.
Comment on attachment 268997 [details] patch Attachment 268997 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/691652 New failing tests: inspector/script-profiler/event-type-API.html inspector/script-profiler/tracking.html inspector/script-profiler/event-type-Microtask.html
Created attachment 269006 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 268997 [details] patch Attachment 268997 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/691646 New failing tests: inspector/script-profiler/tracking.html inspector/script-profiler/event-type-Microtask.html
Created attachment 269008 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 268997 [details] patch Attachment 268997 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/691712 New failing tests: inspector/script-profiler/tracking.html inspector/script-profiler/event-type-Microtask.html
Created attachment 269011 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 268997 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=268997&action=review This looks great! I have mostly style nits and a few questions. We should include at least some basic test in LayoutTests/inspector/script-profiler/ to ensure we get sample data at the end, within the time ranges ScriptProfiler.Event startTime/endTime. r- until test and to see another revision. > Source/JavaScriptCore/inspector/agents/InspectorScriptProfilerAgent.cpp:30 > #include "LegacyProfiler.h" We should remove LegacyProfiler from here now! > Source/JavaScriptCore/inspector/agents/InspectorScriptProfilerAgent.cpp:78 > +#else // ENABLE(SAMPLING_PROFILER) Not sure these comments are necessary for small blocks. > Source/JavaScriptCore/inspector/agents/InspectorScriptProfilerAgent.cpp:105 > +double InspectorScriptProfilerAgent::willEvaluateScript(JSGlobalObject&) Seems we can rip out the JSGlobalObject argument in these methods. > Source/JavaScriptCore/inspector/agents/InspectorScriptProfilerAgent.cpp:159 > + Ref<Protocol::Array<Protocol::ScriptProfiler::StackTrace>> stackTraces = Protocol::Array<Protocol::ScriptProfiler::StackTrace>::create(); > + for (SamplingProfiler::StackTrace& stackTrace : samplingProfilerStackTraces) { > + Ref<Protocol::Array<Protocol::ScriptProfiler::StackFrame>> frames = Protocol::Array<Protocol::ScriptProfiler::StackFrame>::create(); These array variables would be good use of `auto` to avoid repeating the long names. > Source/JavaScriptCore/inspector/agents/InspectorScriptProfilerAgent.cpp:161 > + RefPtr<Protocol::ScriptProfiler::StackFrame> frame = Protocol::ScriptProfiler::StackFrame::create() I think you can just do a Ref<> here instead of RefPtr<>. > Source/JavaScriptCore/inspector/agents/InspectorScriptProfilerAgent.cpp:189 > + if (SamplingProfiler* samplingProfiler = m_environment.scriptDebugServer().vm().samplingProfiler()) { We only want to stop a sampler if we started it. So we should probably have a member variable `m_sampling` and take this branch in that case. > Source/JavaScriptCore/inspector/agents/InspectorScriptProfilerAgent.cpp:197 > +#endif // ENABLE(SAMPLING_PROFILER) And if we didn't include samples, we can just send null, if we make `samples` optional in the event. m_frontendDispatcher->trackingComplete(nullptr) > Source/JavaScriptCore/inspector/protocol/ScriptProfiler.json:34 > + { "name": "timestamp", "type": "number", "description": "Time since the start of recording." }, Is this the timestamp since the start of the recording, or just the timestamp via the stopwatch? > Source/JavaScriptCore/inspector/protocol/ScriptProfiler.json:42 > + { "name": "totalTime", "type": "number", "description": "Total time spent profiling in microseconds." }, I wonder if startTime/endTime would be more appropriate than totalTime. Or, we can determine a startTime from the first sample and endTime from the last. Is this really in microseconds? > Source/JavaScriptCore/inspector/protocol/ScriptProfiler.json:79 > + { "name": "samples", "$ref": "Samples", "description": "Stack traces." } We can make this optional. > Source/JavaScriptCore/runtime/SamplingProfiler.cpp:515 > + return ""; Lets prefer String() for the empty string. > Source/JavaScriptCore/runtime/SamplingProfiler.cpp:544 > return ""; Lets prefer String() for the empty string. > Source/JavaScriptCore/runtime/SamplingProfiler.cpp:595 > + String url = static_cast<ScriptExecutable*>(executable)->sourceURL(); In all of this code, is it guaranteed that the executable is a ScriptExecutable? Should we jsDynamicCast<> to be safe? > Source/JavaScriptCore/runtime/SamplingProfiler.cpp:612 > String SamplingProfiler::stacktracesAsJSON() Style: Consistency "stacktracesAsJSON" => "stackTracesAsJSON". Depending on performance, something we've considered is the backend sending a raw JSON string to the frontend instead of building Inspector::Protocol objects, then JSON stringifying them. For something like this, that may be useful to save memory and time when building the trackingComplete response message. I think for now what you have is fine (and preferred), but something to consider in the future if we encounter issues. > Source/JavaScriptCore/runtime/SamplingProfiler.h:78 > + String displayName(); > + String displayNameForJSONTests(); I don't understand why these need to be different. Perhaps there should be a comment here explaining why the test version is needed. I think they can be merged, unless I overlooked something. I'd hate for something to be different in tests then in the real world unless it is really needed. > Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:687 > + scriptProfilerTrackingCompleted(/*Protocol::ScriptProfiler::Samples*/ samples) We tend to avoid such comments. Do they help you? > Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:-690 > - // Associate the profiles with the ScriptProfiler created records. > - if (profiles) { If we make `samples` optional, we should have `if (samples)` here. > Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:693 > + this._callingContextTree = new WebInspector.CallingContextTree(); Style: Our style is to leave off optional "()" on construction: this._callingContextTree = new WebInspector.CallingContextTree; > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:32 > + this._root = new WebInspector.CCTNode(-1, -1, -1, "<root>", "I.am.not.a.real.url"); I'd suggest passing `null` for the URL instead of this made up value. > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:108 > + for (let i = 0; i < this._timestamps.length; i++) { > + let timestamp = this._timestamps[i]; > + if (timestamp >= startTime && timestamp <= endTime) > + return true; > + } > + return false; > + } We may want to consider a binary search. Which would require keeping this._timestamps in a sorted order. I believe that is already the case here. > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:114 > + return this._timestamps.filter(function(timestamp) { > + return timestamp >= startTime && timestamp <= endTime; > + }); Style: Feel free to use error functions for trivial cases like this. > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:131 > + addTimestamp(timestamp) > + { > + this._timestamps.push(timestamp); > + } It may be useful to keep timestamps in a sorted order. We can assert here: console.assert(!this._timestamps.length || this._timestamps.lastValue < timestamp, "Expected timestamps to be added in sorted, increasing, order."); > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:177 > + forEachNode(callback) Style: Having this next to forEachChild would be nice. > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:190 > + static _hash(/*Protocol::Timeline::StackFrame*/ stackFrame) > + { > + return stackFrame.name + ":" + stackFrame.sourceID + ":" + stackFrame.line + ":" + stackFrame.column; > + } Style: We prefer putting static functions between the Constructor and Public methods. Easy to search for. > Source/WebInspectorUI/UserInterface/Views/ScriptTimelineView.js:41 > + var isSamplingProfiler = !!window.ScriptProfilerAgent; Style: let.
Comment on attachment 268997 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=268997&action=review >> Source/JavaScriptCore/inspector/agents/InspectorScriptProfilerAgent.cpp:30 >> #include "LegacyProfiler.h" > > We should remove LegacyProfiler from here now! done. >> Source/JavaScriptCore/inspector/agents/InspectorScriptProfilerAgent.cpp:78 >> +#else // ENABLE(SAMPLING_PROFILER) > > Not sure these comments are necessary for small blocks. I kept it on the #endif but removed it from the #else. I think even though it's not super helpful now, it prevents future unwieldiness. I've seen files start off small and get worse over time with the legibility of the #ifdefs. >> Source/JavaScriptCore/inspector/agents/InspectorScriptProfilerAgent.cpp:105 >> +double InspectorScriptProfilerAgent::willEvaluateScript(JSGlobalObject&) > > Seems we can rip out the JSGlobalObject argument in these methods. done. >> Source/JavaScriptCore/inspector/agents/InspectorScriptProfilerAgent.cpp:159 >> + Ref<Protocol::Array<Protocol::ScriptProfiler::StackFrame>> frames = Protocol::Array<Protocol::ScriptProfiler::StackFrame>::create(); > > These array variables would be good use of `auto` to avoid repeating the long names. I think it's helpful to see the type written out when you end up WTFMove()ing the thing. >> Source/JavaScriptCore/inspector/agents/InspectorScriptProfilerAgent.cpp:161 >> + RefPtr<Protocol::ScriptProfiler::StackFrame> frame = Protocol::ScriptProfiler::StackFrame::create() > > I think you can just do a Ref<> here instead of RefPtr<>. done. >> Source/JavaScriptCore/inspector/protocol/ScriptProfiler.json:34 >> + { "name": "timestamp", "type": "number", "description": "Time since the start of recording." }, > > Is this the timestamp since the start of the recording, or just the timestamp via the stopwatch? stopwatch. >> Source/JavaScriptCore/inspector/protocol/ScriptProfiler.json:42 >> + { "name": "totalTime", "type": "number", "description": "Total time spent profiling in microseconds." }, > > I wonder if startTime/endTime would be more appropriate than totalTime. Or, we can determine a startTime from the first sample and endTime from the last. > > Is this really in microseconds? I don't think startTime/endTime works here. Even though we're not using the totalTime field, the sampling profiler tries to accumulate a sensible number for totalTime. totalTime is an estimate of execution time, not total elapsed time. So if you had a sample for 10 seconds, but code only executed for 5 seconds, the totalTime should be 5 seconds here. It's not microseconds. Fixed. >> Source/JavaScriptCore/inspector/protocol/ScriptProfiler.json:79 >> + { "name": "samples", "$ref": "Samples", "description": "Stack traces." } > > We can make this optional. done. >> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:595 >> + String url = static_cast<ScriptExecutable*>(executable)->sourceURL(); > > In all of this code, is it guaranteed that the executable is a ScriptExecutable? Should we jsDynamicCast<> to be safe? Yeah we're guaranteed this. The only type of ExecutableBase that isn't a ScriptExecutable is a NativeExecutable >> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:612 >> String SamplingProfiler::stacktracesAsJSON() > > Style: Consistency "stacktracesAsJSON" => "stackTracesAsJSON". > > Depending on performance, something we've considered is the backend sending a raw JSON string to the frontend instead of building Inspector::Protocol objects, then JSON stringifying them. For something like this, that may be useful to save memory and time when building the trackingComplete response message. I think for now what you have is fine (and preferred), but something to consider in the future if we encounter issues. Ok. If we do that, that would actually guarantee that JSC stress tests will be testing the same JSON blob as the inspector, so that might be another incentive to do this. >> Source/JavaScriptCore/runtime/SamplingProfiler.h:78 >> + String displayNameForJSONTests(); > > I don't understand why these need to be different. Perhaps there should be a comment here explaining why the test version is needed. I think they can be merged, unless I overlooked something. I'd hate for something to be different in tests then in the real world unless it is really needed. I'll add a comment. The JSC JSON version will return "(anonymous function)" instead of "". It will return "(eval)" instead of "(program)" for eval'd code. >> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:687 >> + scriptProfilerTrackingCompleted(/*Protocol::ScriptProfiler::Samples*/ samples) > > We tend to avoid such comments. Do they help you? Nope. removed. >> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:-690 >> - if (profiles) { > > If we make `samples` optional, we should have `if (samples)` here. done. >> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:693 >> + this._callingContextTree = new WebInspector.CallingContextTree(); > > Style: Our style is to leave off optional "()" on construction: > > this._callingContextTree = new WebInspector.CallingContextTree; done. >> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:108 >> + } > > We may want to consider a binary search. Which would require keeping this._timestamps in a sorted order. I believe that is already the case here. done. >> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:114 >> + }); > > Style: Feel free to use error functions for trivial cases like this. done. >> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:131 >> + } > > It may be useful to keep timestamps in a sorted order. We can assert here: > > console.assert(!this._timestamps.length || this._timestamps.lastValue < timestamp, "Expected timestamps to be added in sorted, increasing, order."); done. >> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:177 >> + forEachNode(callback) > > Style: Having this next to forEachChild would be nice. done. >> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:190 >> + } > > Style: We prefer putting static functions between the Constructor and Public methods. Easy to search for. done. >> Source/WebInspectorUI/UserInterface/Views/ScriptTimelineView.js:41 >> + var isSamplingProfiler = !!window.ScriptProfilerAgent; > > Style: let. done.
Created attachment 269120 [details] patch
Created attachment 269123 [details] patch
Comment on attachment 269123 [details] patch Attachment 269123 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/696290 New failing tests: imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-collection.html
Created attachment 269133 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 269123 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=269123&action=review Looks good. I have to do a more detailed final review of some parts before a final r+. Someone else might beat me to it. > Source/JavaScriptCore/inspector/agents/InspectorScriptProfilerAgent.cpp:208 > + m_frontendDispatcher->trackingComplete(nullptr); Style: dedent. > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:88 > + if (stackNode[propertyName] != node[propertyName]) Style: !== > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:89 > + continue outer; Heh! We haven't used a label anywhere else in our JavaScript (but CodeMirror does use it). > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:168 > + let found = false; > + let lower = 0; > + let upper = length - 1; > + while (true) { > + let mid = Math.floor((lower + upper) / 2); > + let item = timestamps[mid]; > + if (startTime <= item && item <= endTime) { > + found = true; > + break; > + } > + > + if (endTime < item) > + upper = mid - 1; > + else > + lower = mid + 1; > + > + if (lower > upper) > + break; > + } I think you can use our Array functions in Utilities.js. Specifically lowerBound to binary search the "endTime", and then check the timestamp. let index = timestamps.lowerBound(endTime); if (index === timestamps.length) return false; let item = timestamps[index]; return startTime <= item && item <= endTime; Or you can keep what you have, which is technically nicer since it bails as soon as it has something in the range. > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:171 > + console.assert(!(!found && this.filteredTimestamps(startTime, endTime).length)); > + console.assert(!(found && !this.filteredTimestamps(startTime, endTime).length)); We got rid of a linear search and then added a linear search assert! =( > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:181 > + filteredTimestamps(startTime, endTime) > + { > + return this._timestamps.filter((timestamp) => { > + return timestamp >= startTime && timestamp <= endTime; > + }); > + } This could also also use a binary search. Do you think it is worth it? Binary search to find the first record in the range. Walk records until we are out of range. Done. > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:201 > + console.assert(!this._timestamps.length || this._timestamps[this._timestamps.length - 1] <= timestamp, "Expected timestamps to be added in sorted, increasing, order."); Nit: Utilities.js adds Array.prototype.lastValue to simplify this. > LayoutTests/inspector/sampling-profiler/basic.html:30 > + test: function(resolve, reject) { Style: We are almost exclusively using Arrow functions in tests where possible, like here! > LayoutTests/inspector/sampling-profiler/basic.html:37 > + { > + let stackTraces = messageObject.params.samples.stackTraces; > + for (let i = 0; i < stackTraces.length; i++) > + tree.updateTreeWithStackTrace(stackTraces[i]); > + } I don't see a need to create the extra block in these tests? Especially given let's semantics it should be fine to have this without the anonymous block. > LayoutTests/inspector/sampling-profiler/call-frame-with-dom-functions.html:16 > + let body = document.getElementsByTagName("body")[0]; Nit: document.body > LayoutTests/inspector/sampling-profiler/call-frame-with-dom-functions.html:26 > + let suite = ProtocolTest.createAsyncSuite("SamplingProfiler.basic"); We have been preferring each test have a unique name. "SamplingProfiler.basic" is used in all of these tests. I would have named these something like: ScriptProfiler.Samples.Basic ScriptProfiler.Samples.DOM ScriptProfiler.Samples.SourceURL ScriptProfiler.Samples.Deep > LayoutTests/inspector/sampling-profiler/call-frame-with-dom-functions.html:58 > + ProtocolTest.evaluateInPage("runFor(foo, 600)"); What determines how long you've written the runFor for? Most are 400, this one is 600. That means this test is guaranteed to take more than half a second. Could we drop these down to 50 or 100 and would they still pass reliably?
Comment on attachment 269123 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=269123&action=review Thanks for your review, I'll upload another patch sometime this weekend. >> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:88 >> + if (stackNode[propertyName] != node[propertyName]) > > Style: !== Will change >> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:168 >> + } > > I think you can use our Array functions in Utilities.js. Specifically lowerBound to binary search the "endTime", and then check the timestamp. > > let index = timestamps.lowerBound(endTime); > if (index === timestamps.length) > return false; > > let item = timestamps[index]; > return startTime <= item && item <= endTime; > > Or you can keep what you have, which is technically nicer since it bails as soon as it has something in the range. I'll look into your suggested method. It's probably better to have code re-use. Or, if not, I can add a binary search method that does this range-based approach, it might be useful elsewhere. >> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:171 >> + console.assert(!(found && !this.filteredTimestamps(startTime, endTime).length)); > > We got rid of a linear search and then added a linear search assert! =( Well, this shouldn't show up in production builds. I wish we had a way to have asserts in the inspector only for debug builds (maybe there is, and I don't know about it?). But I think this is a worthy assertion because it makes sure our algorithm isn't full of crap, and it won't penalize performance on production builds. >> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:181 >> + } > > This could also also use a binary search. Do you think it is worth it? Binary search to find the first record in the range. Walk records until we are out of range. Done. I'll look into doing it this way. This could also do some smart things where it checks if: startTime <= firstItem and endTime >= endItem (which seems probable for many situations) and then just return the whole array. >> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:201 >> + console.assert(!this._timestamps.length || this._timestamps[this._timestamps.length - 1] <= timestamp, "Expected timestamps to be added in sorted, increasing, order."); > > Nit: Utilities.js adds Array.prototype.lastValue to simplify this. will change to be this. >> LayoutTests/inspector/sampling-profiler/basic.html:30 >> + test: function(resolve, reject) { > > Style: We are almost exclusively using Arrow functions in tests where possible, like here! Ok. I'll switch. FWIW, arrow functions, unfortunately, are still slower than regular functions in the situation where function doesn't close over any variables. So this doesn't matter here (because we don't close over variables, and because it's a test) , but there might be places in the code base where it is currently valuable to not use arrow functions when we don't close over any variables. That said, we are working on making arrow functions have no overhead in the common case. >> LayoutTests/inspector/sampling-profiler/basic.html:37 >> + } > > I don't see a need to create the extra block in these tests? Especially given let's semantics it should be fine to have this without the anonymous block. I just like to have things go out of scope when they're not longer needed to prevent any mistakes. So in this specific block, stackTraces is only visible inside the "{}" block. That said, this code is duplicated across all tests. That's a good sign I should just create a CallingContextTree.__test_makeTreeFromProtocolMessageObject() function. >> LayoutTests/inspector/sampling-profiler/call-frame-with-dom-functions.html:16 >> + let body = document.getElementsByTagName("body")[0]; > > Nit: document.body Will change >> LayoutTests/inspector/sampling-profiler/call-frame-with-dom-functions.html:26 >> + let suite = ProtocolTest.createAsyncSuite("SamplingProfiler.basic"); > > We have been preferring each test have a unique name. "SamplingProfiler.basic" is used in all of these tests. > > I would have named these something like: > > ScriptProfiler.Samples.Basic > ScriptProfiler.Samples.DOM > ScriptProfiler.Samples.SourceURL > ScriptProfiler.Samples.Deep Will change. >> LayoutTests/inspector/sampling-profiler/call-frame-with-dom-functions.html:58 >> + ProtocolTest.evaluateInPage("runFor(foo, 600)"); > > What determines how long you've written the runFor for? Most are 400, this one is 600. That means this test is guaranteed to take more than half a second. Could we drop these down to 50 or 100 and would they still pass reliably? I'm not sure how to gauge reliability here. I'll drop these numbers as low as I can locally, and then we can gauge the reliability with the bots. If tests are failing from not running long enough, we can bump up the number.
Comment on attachment 269123 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=269123&action=review >>> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:168 >>> + } >> >> I think you can use our Array functions in Utilities.js. Specifically lowerBound to binary search the "endTime", and then check the timestamp. >> >> let index = timestamps.lowerBound(endTime); >> if (index === timestamps.length) >> return false; >> >> let item = timestamps[index]; >> return startTime <= item && item <= endTime; >> >> Or you can keep what you have, which is technically nicer since it bails as soon as it has something in the range. > > I'll look into your suggested method. It's probably better to have code re-use. > Or, if not, I can add a binary search method that does this range-based approach, it might > be useful elsewhere. After thinking about this, we really want to run "lowerBound" on lowerBound(startTime) not lowerBound(endTime) Having lowerBound(endTime) doesn't tell us about a matching timestamp because endTime might be greater than everything in the list. That's fine, but it doesn't mean that startTime might not be smaller than *something* in the list.
Created attachment 269245 [details] patch
Created attachment 269246 [details] patch
Created attachment 269248 [details] patch
Comment on attachment 269248 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=269248&action=review r=me, Looks good to me. I would recommend updating this variable in ScriptInstrument.js before landing: // FIXME: Make this some UI visible option. const includeProfiles = true; ScriptProfilerAgent.startTracking(includeProfiles); From "includeProfiles" to "includeSamples". > Source/JavaScriptCore/inspector/agents/InspectorScriptProfilerAgent.cpp:185 > + Ref<Protocol::ScriptProfiler::Samples> result = Protocol::ScriptProfiler::Samples::create() > + .setStackTraces(WTFMove(stackTraces)) > .setTotalTime(totalTime) > .release(); You could just `return` this object without storing it inso the local, "result". > Source/JavaScriptCore/inspector/agents/InspectorScriptProfilerAgent.cpp:202 > + locker.unlockEarly(); Neat! > Source/JavaScriptCore/inspector/agents/InspectorScriptProfilerAgent.cpp:209 > + Nit: Unnecessary empty line. > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:46 > + let timestamp = stackTrace.timestamp; > + this._totalNumberOfSamples++; > + let node = this._root; > + node.addTimestamp(timestamp); > + let stackFrames = stackTrace.stackFrames; Style: You could combine these two lines into one destructuring line at the top: let {timestamp, stackFrames} = stackTrace; > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:166 > + console.assert(!(!hasTimestampInRange && this.filteredTimestamps(startTime, endTime).length)); > + console.assert(!(hasTimestampInRange && !this.filteredTimestamps(startTime, endTime).length)); I'm not sure this is worth keeping these asserts. It pains me to see asserts like this that might have O(n) runtime which might have unexpected runtime costs. > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:177 > + if (!(startTime <= timestamp && timestamp <= endTime)) The startTime <= timestamp part should always be true right? So it could be removed?
Comment on attachment 269248 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=269248&action=review > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:88 > + // We don't look for a match that traces upwards to the root, but we match by This comment needs help. I'll ipdate before landing
Comment on attachment 269248 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=269248&action=review >> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:46 >> + let stackFrames = stackTrace.stackFrames; > > Style: You could combine these two lines into one destructuring line at the top: > > let {timestamp, stackFrames} = stackTrace; Done. I just made the parameter to the function the destructuring pattern. >> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:166 >> + console.assert(!(hasTimestampInRange && !this.filteredTimestamps(startTime, endTime).length)); > > I'm not sure this is worth keeping these asserts. It pains me to see asserts like this that might have O(n) runtime which might have unexpected runtime costs. removed. >> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:177 >> + if (!(startTime <= timestamp && timestamp <= endTime)) > > The startTime <= timestamp part should always be true right? So it could be removed? Yeah, you're correct. I've just made it an assert.
Created attachment 269358 [details] patch for landing.
Comment on attachment 269358 [details] patch for landing. Clearing flags on attachment: 269358 Committed r195376: <http://trac.webkit.org/changeset/195376>
All reviewed patches have been landed. Closing bug.
It looks like this change broke the CLoop build: <https://build.webkit.org/builders/Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29/builds/2656> <https://build.webkit.org/builders/Apple%20Yosemite%20LLINT%20CLoop%20%28BuildAndTest%29/builds/12400> Source/JavaScriptCore/inspector/agents/InspectorScriptProfilerAgent.h:75:10: error: private field 'm_enabledSamplingProfiler' is not used [-Werror,-Wunused-private-field]
(In reply to comment #40) > It looks like this change broke the CLoop build: > <https://build.webkit.org/builders/ > Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29/builds/2656> > <https://build.webkit.org/builders/ > Apple%20Yosemite%20LLINT%20CLoop%20%28BuildAndTest%29/builds/12400> > > Source/JavaScriptCore/inspector/agents/InspectorScriptProfilerAgent.h:75:10: > error: private field 'm_enabledSamplingProfiler' is not used > [-Werror,-Wunused-private-field] Saam landed a build fix: <http://trac.webkit.org/changeset/195383>