Bug 163463

Summary: Web Inspector: Heatmap Profiler: Highlight function definitions
Product: WebKit Reporter: Nikita Vasilyev <nvasilyev>
Component: Web InspectorAssignee: Nikita Vasilyev <nvasilyev>
Status: NEW    
Severity: Normal CC: inspector-bugzilla-changes, mattbaker, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[Image] WIP
none
Patch
nvasilyev: review-, nvasilyev: commit-queue-
[Image] With patch applied
none
[Patch] WIP
nvasilyev: review-, nvasilyev: commit-queue-
Patch
timothy: review-
Patch
timothy: review-
[Image] Heatmap of HeatmapAnnotator
none
[Image] Time Duration Tokens none

Nikita Vasilyev
Reported 2016-10-14 15:55:18 PDT
Created attachment 291673 [details] [Image] WIP As a first minimum viable product, highlight only function definitions (function expressions, class methods, arrow functions, etc.) and nothing inside the function or expressions in the global scope.
Attachments
[Image] WIP (219.33 KB, image/png)
2016-10-14 15:55 PDT, Nikita Vasilyev
no flags
Patch (38.60 KB, patch)
2016-10-15 13:13 PDT, Nikita Vasilyev
nvasilyev: review-
nvasilyev: commit-queue-
[Image] With patch applied (135.68 KB, image/png)
2016-10-17 11:17 PDT, Nikita Vasilyev
no flags
[Patch] WIP (15.71 KB, patch)
2016-10-17 11:19 PDT, Nikita Vasilyev
nvasilyev: review-
nvasilyev: commit-queue-
Patch (17.58 KB, patch)
2016-10-17 15:37 PDT, Nikita Vasilyev
timothy: review-
Patch (16.00 KB, patch)
2016-11-09 11:50 PST, Nikita Vasilyev
timothy: review-
[Image] Heatmap of HeatmapAnnotator (129.52 KB, image/png)
2016-11-11 16:32 PST, Nikita Vasilyev
no flags
[Image] Time Duration Tokens (95.22 KB, image/png)
2016-12-19 15:43 PST, Nikita Vasilyev
no flags
Radar WebKit Bug Importer
Comment 1 2016-10-14 15:56:12 PDT
Nikita Vasilyev
Comment 2 2016-10-15 13:13:14 PDT
Created attachment 291722 [details] Patch Reduction: http://nv.github.io/webkit-inspector-bugs/163463_heatmap/ With this WIP patch applied, you should see the same as on the attached screenshot.
Nikita Vasilyev
Comment 3 2016-10-17 11:17:28 PDT
Created attachment 291837 [details] [Image] With patch applied
Nikita Vasilyev
Comment 4 2016-10-17 11:19:13 PDT
Created attachment 291838 [details] [Patch] WIP
Nikita Vasilyev
Comment 5 2016-10-17 11:24:07 PDT
(In reply to comment #4) > Created attachment 291838 [details] > [Patch] WIP Things to fix: 1. Arrow functions aren't highlighted. 2. For: blah() { ... } The duration token is placed on the line below the method name: blah() [6.31ms] { ... }
Nikita Vasilyev
Comment 6 2016-10-17 15:37:30 PDT
Nikita Vasilyev
Comment 7 2016-10-17 15:39:48 PDT
(In reply to comment #5) > (In reply to comment #4) > > Created attachment 291838 [details] > > [Patch] WIP > > Things to fix: > > 1. Arrow functions aren't highlighted. > > 2. For: > > blah() > { > ... > } > > The duration token is placed on the line below the method name: > > blah() > [6.31ms] { > ... > } Neither of these addressed in the patch above. I'm going to fix it in the follow up patch.
Devin Rousso
Comment 8 2016-10-18 16:41:53 PDT
Comment on attachment 291884 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291884&action=review > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:49 > + // FIXME: Use "for-of" loop once it's as fast as a "for" loop. Really? Where (and how) did you test this? > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:55 > + if (visited.has(node)) NIT: you could merge this with the previous `if`. > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:82 > + NIT: extra space. > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:113 > + css: "background: " + cssColorValue, NIT: template string? > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:132 > + const NodeType = WebInspector.ScriptSyntaxTree.NodeType; NIT: I would say that if you plan on making this "alias", it should be placed at the top of the function (above all logic). > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:134 > + let rangeForFunction = (token) => { NIT: this could just be a regular function, since the context isn't needed. > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:139 > + // function foo() {} NIT: I think you should clarify this comment by saying "this is an example". > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:143 > + // class AppClass() {constructor() {}} Putting comments in between "if...else" seems very awkward. > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:129 > + let visited = new Set; It seems like you aren't ever adding to `visited`. Wouldn't it be easier to just use `visited` and `Array.from`? let visited = new Set; this.forEachNode((node) => visited.add(node)}); return Array.from(visited); > Source/WebInspectorUI/UserInterface/Models/CallingContextTreeNode.js:71 > + let i = this._durations.length; Is there a reason why you need to start at the end and iterate backwards? Could you just use `reduce`? return this._durations.reduce((previousValue, currentValue) => previousValue + currentValue); > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:54 > + findTokenAtOffset(offset) This function seems very complicated and could use some more in-depth comments. Also, I feel like it may be cleaner to make local functions for each node type to let you use early returns. > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:59 > + for (let i = 0; i < body.length; i++) { NIT: `for...of` instead?
Timothy Hatcher
Comment 9 2016-11-08 09:51:22 PST
Comment on attachment 291884 [details] Patch Based on feedback from the contributors meeting, we should highlight all ranges, not just function calls.
Nikita Vasilyev
Comment 10 2016-11-08 13:19:21 PST
(In reply to comment #9) > Comment on attachment 291884 [details] > Patch > > Based on feedback from the contributors meeting, we should highlight all > ranges, not just function calls. Yes, we should highlight all ranges. The patch above is a stepping stone to this. I'll post an updated patch later today.
Nikita Vasilyev
Comment 11 2016-11-08 13:50:58 PST
(In reply to comment #8) > Comment on attachment 291884 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=291884&action=review > > > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:49 > > + // FIXME: Use "for-of" loop once it's as fast as a "for" loop. > > Really? Where (and how) did you test this? A "for" loop is >20 times faster that a "for-of" loop. https://jsperf.com/for-of-vs-for-loops Generally, operations inside the loop are performance sensitive, not the loop code itself. However, we use "for" loops in code that runs very often, such as WebInspector.Object#dispatchEventToListeners. > > > Source/WebInspectorUI/UserInterface/Models/CallingContextTreeNode.js:71 > > + let i = this._durations.length; > > Is there a reason why you need to start at the end and iterate backwards? > Could you just use `reduce`? `reduce` is slower. Again, this is a micro optimization which is only noticeable in a "hot" code.
Nikita Vasilyev
Comment 12 2016-11-08 17:02:30 PST
(In reply to comment #8) > Comment on attachment 291884 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=291884&action=review > > > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:143 > > + // class AppClass() {constructor() {}} > > Putting comments in between "if...else" seems very awkward. Any suggestions on where to put them? > > > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:129 > > + let visited = new Set; > > It seems like you aren't ever adding to `visited`. Wouldn't it be easier to > just use `visited` and `Array.from`? > > let visited = new Set; > this.forEachNode((node) => visited.add(node)}); > return Array.from(visited); Array.from(aSet) is at least 4x slower. https://jsperf.com/array-from-set-vs-array-push/1
Nikita Vasilyev
Comment 13 2016-11-09 11:50:13 PST
Timothy Hatcher
Comment 14 2016-11-11 10:15:46 PST
Comment on attachment 294258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294258&action=review > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:43 > + // FIXME: >10k nodes can be slow to process. > + let nodes = WebInspector.timelineManager.activeRecording.topFunctionsBottomUpCallingContextTree.toFlatList(); This does seem like it would be very expensive. Why does it need flattened first? Flattening it seems like it would make it harder to fi the other FIXME in this patch that only annotates the viewport lines. > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:75 > + clearAnnotations() > + { > + // No-op > + } Why a no-op? > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:82 > + Nit: Extra line. > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:83 > + if (node.name === "(program)") Is there not a node type for this instead of checking for the name? > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:87 > + let token = syntaxTree.findTokenAtOffset(offset); This is very expensive! _highlightNodes is called in a loop, and findTokenAtOffset does two expensive walks on the nodes. > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:126 > + _createDurationWidget(totalDuration) > + { > + let widgetElement = document.createElement("span"); > + widgetElement.classList.add("duration-token"); > + widgetElement.textContent = Number.secondsToString(totalDuration); > + return widgetElement; > + } I don't think the durations widget holds up well once we start highlighting all ranges, not just function calls. It would be too noisy. What other design should be considered for this info? > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:133 > + console.warn("Visited:", node); What is this warning about? > Source/WebInspectorUI/UserInterface/Models/CallingContextTreeNode.js:43 > + // FIXME: Store leaf node objects in an array. Why? > Source/WebInspectorUI/UserInterface/Models/CallingContextTreeNode.js:74 > + Nit: Can remove this blank line. > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:67 > + let containers = this.containersOfOffset(offset); > + let index = containers.length; > + let container; > + > + while (index--) { > + container = containers[index]; > + if (supportedContainerTypes.has(container.type)) > + break; > + } This is an expensive operation. The containersOfOffset function already walks all the nodes. Then you walk them a second time. Perhaps containersOfOffset should have some options to support what you need during a single node walk? > Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:359 > + markText(startPosition, endPosition, options) > + { > + this._codeMirror.getDoc().markText(startPosition, endPosition, options); > + } _codeMirror is private to WebInspector.TextEditor, and should not be used here. This markText fiction should move to TextEditor.js. This also leaks the underlying details of CodeMirror to the callers of this by just blindly passing options through. If we want to do this, then we would want an object or API for our own options that we could map to CodeMirror or other text editor in the future.
Nikita Vasilyev
Comment 15 2016-11-11 16:32:26 PST
Created attachment 294562 [details] [Image] Heatmap of HeatmapAnnotator I profiled HeatmapAnnotator.js using Heatmap Profiler (how cool is that?!). Some things you can see: 1. toFlatList is very fast relative to _highlightNodes (123 times faster). 2. _highlightNodes is primarily slow because of setInlineWidget. 3. findTokenAtOffset fairly fast, 14 times faster than setInlineWidget. This is setInlineWidget: setInlineWidget(position, inlineElement) { return this._codeMirror.setUniqueBookmark(position, {widget: inlineElement}); } I don't think I can make setUniqueBookmark faster, but if I only render widgets that are in the viewport that shouldn't be needed. Please note, to take this screenshot I used a version of Heatmap Profiler that highlights all expressions, not just function definitions. It's different from the one in the attached patch but HeatmapAnnotator class is almost the same.
Nikita Vasilyev
Comment 16 2016-11-11 16:40:29 PST
(In reply to comment #14) > > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:126 > > + _createDurationWidget(totalDuration) > > + { > > + let widgetElement = document.createElement("span"); > > + widgetElement.classList.add("duration-token"); > > + widgetElement.textContent = Number.secondsToString(totalDuration); > > + return widgetElement; > > + } > > I don't think the durations widget holds up well once we start highlighting > all ranges, not just function calls. It would be too noisy. What other > design should be considered for this info? 1. Only show duration widgets for function definitions. 2. Only show duration widgets for 20% slowest nodes. (I like this one!)
Nikita Vasilyev
Comment 17 2016-11-12 13:53:57 PST
(In reply to comment #14) > Comment on attachment 294258 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=294258&action=review > > > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:43 > > + // FIXME: >10k nodes can be slow to process. > > + let nodes = WebInspector.timelineManager.activeRecording.topFunctionsBottomUpCallingContextTree.toFlatList(); > > This does seem like it would be very expensive. Why does it need flattened > first? Flattening it seems like it would make it harder to fi the other > FIXME in this patch that only annotates the viewport lines. I should remove this FIXME, it actually not very expensive as noted above in comment #c15. > > > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:75 > > + clearAnnotations() > > + { > > + // No-op > > + } > > Why a no-op? I'll remove it. > > > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:82 > > + > > Nit: Extra line. > > > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:83 > > + if (node.name === "(program)") > > Is there not a node type for this instead of checking for the name? There's currently nothing like node.type. It does look a bit dirty. Maybe we can introduce "isProgram" getter. > > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:133 > > + console.warn("Visited:", node); > > What is this warning about? This should be removed. I was making sure there are not duplicate nodes. > > > Source/WebInspectorUI/UserInterface/Models/CallingContextTreeNode.js:43 > > + // FIXME: Store leaf node objects in an array. > > Why? To highlight leaf nodes (all expressions). > > > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:67 > > + let containers = this.containersOfOffset(offset); > > + let index = containers.length; > > + let container; > > + > > + while (index--) { > > + container = containers[index]; > > + if (supportedContainerTypes.has(container.type)) > > + break; > > + } > > This is an expensive operation. The containersOfOffset function already > walks all the nodes. Then you walk them a second time. Perhaps > containersOfOffset should have some options to support what you need during > a single node walk? It isn't the most expensive operation in my patch, as I showed above, yet I agree this can be optimized to do only a single node walk. > > > Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:359 > > + markText(startPosition, endPosition, options) > > + { > > + this._codeMirror.getDoc().markText(startPosition, endPosition, options); > > + } > > _codeMirror is private to WebInspector.TextEditor, and should not be used > here. This markText fiction should move to TextEditor.js. > > This also leaks the underlying details of CodeMirror to the callers of this > by just blindly passing options through. If we want to do this, then we > would want an object or API for our own options that we could map to > CodeMirror or other text editor in the future. You're right, I was a bit sloppy here.
Nikita Vasilyev
Comment 18 2016-11-27 18:40:28 PST
(In reply to comment #14) > > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:67 > > + let containers = this.containersOfOffset(offset); > > + let index = containers.length; > > + let container; > > + > > + while (index--) { > > + container = containers[index]; > > + if (supportedContainerTypes.has(container.type)) > > + break; > > + } > > This is an expensive operation. The containersOfOffset function already > walks all the nodes. Then you walk them a second time. Perhaps > containersOfOffset should have some options to support what you need during > a single node walk? I walk nodes back to find the innermost node of a certain type. I can't exit early on the first occurrence of a supported type, I have to backtrack. Consider the following example: function foo() { function bar|() {return 42} return bar() } (| is the offset position) containersOfOffset sees foo's FunctionDeclaration before bar's FunctionDeclaration. If I exit early on the first occurrence of FunctionDeclaration, I'd get foo() and not bar().
Nikita Vasilyev
Comment 19 2016-12-19 15:43:17 PST
Created attachment 297486 [details] [Image] Time Duration Tokens Redesigned time duration tokens. Inspired by cssarrowplease.com, these are implemented in pure CSS (no images or SVGs). Let me know what you think!
Nikita Vasilyev
Comment 20 2016-12-19 15:47:12 PST
(In reply to comment #19) > Created attachment 297486 [details] > [Image] Time Duration Tokens > > Redesigned time duration tokens. Inspired by cssarrowplease.com, these are > implemented in pure CSS (no images or SVGs). Let me know what you think! Same image: https://cloudup.com/cNNMQCHJHrC (Cloudup, unlike Bugzilla, detects retina images and displays them properly scaled).
Matt Baker
Comment 21 2016-12-19 15:57:18 PST
(In reply to comment #19) > Created attachment 297486 [details] > [Image] Time Duration Tokens > > Redesigned time duration tokens. Inspired by cssarrowplease.com, these are > implemented in pure CSS (no images or SVGs). Let me know what you think! Think these look really nice! It looks like the left-arrow border color is slightly different from the rounded border used by the rest of the token.
Nikita Vasilyev
Comment 22 2016-12-20 11:03:26 PST
(In reply to comment #21) > (In reply to comment #19) > > Created attachment 297486 [details] > > [Image] Time Duration Tokens > > > > Redesigned time duration tokens. Inspired by cssarrowplease.com, these are > > implemented in pure CSS (no images or SVGs). Let me know what you think! > > Think these look really nice! It looks like the left-arrow border color is > slightly different from the rounded border used by the rest of the token. That's intentional. I may look strange on Bugzilla but it looks better on non-zoomed screenshot (https://cloudup.com/cNNMQCHJHrC). I may still make the border and arrow colors more similar.
Note You need to log in before you can comment on or make changes to this bug.