Bug 160973 - Web Inspector: Add a button to navigation bar to toggle Control Flow Profiler
Summary: Web Inspector: Add a button to navigation bar to toggle Control Flow Profiler
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on: 160872
Blocks: 151695
  Show dependency treegraph
 
Reported: 2016-08-18 14:25 PDT by Nikita Vasilyev
Modified: 2016-09-08 16:16 PDT (History)
7 users (show)

See Also:


Attachments
WIP (21.25 KB, patch)
2016-08-18 14:33 PDT, Nikita Vasilyev
nvasilyev: review-
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Patch (25.24 KB, patch)
2016-08-19 13:30 PDT, Nikita Vasilyev
bburg: review-
bburg: commit-queue-
Details | Formatted Diff | Diff
Patch (27.87 KB, patch)
2016-08-28 13:09 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (56.32 KB, patch)
2016-08-28 13:18 PDT, Nikita Vasilyev
bburg: review+
bburg: commit-queue-
Details | Formatted Diff | Diff
Patch (29.79 KB, patch)
2016-09-08 15:38 PDT, Nikita Vasilyev
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch (58.59 KB, patch)
2016-09-08 15:42 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2016-08-18 14:25:37 PDT
The backend of Control Flow Profiler was decoupled from Type Profiler (bug 160750).

Add a button to toggle Control Flow Profiler independently from Type Profiler.
The existing Type Profiler button shouldn't affect Control Flow Profiler any longer.
Comment 1 Radar WebKit Bug Importer 2016-08-18 14:25:57 PDT
<rdar://problem/27912606>
Comment 2 Nikita Vasilyev 2016-08-18 14:33:02 PDT
Created attachment 286397 [details]
WIP

No new icon yet. The same [T] icon is temporary used for Control Flow Profiler.
Comment 3 Joseph Pecoraro 2016-08-18 14:38:49 PDT
Comment on attachment 286397 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=286397&action=review

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:294
> +    toggleDeadCodeHighlights()

This name doesn't sound right. This is showing code coverage, code that has executed or not executed. "Dead Code" is a term of art for unreachable code, and that isn't what we are doing here, right?
Comment 4 Nikita Vasilyev 2016-08-18 14:40:46 PDT
(In reply to comment #3)
> Comment on attachment 286397 [details]
> WIP
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=286397&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:294
> > +    toggleDeadCodeHighlights()
> 
> This name doesn't sound right. This is showing code coverage, code that has
> executed or not executed. "Dead Code" is a term of art for unreachable code,
> and that isn't what we are doing here, right?

You're right. I'll change it.
Comment 5 Nikita Vasilyev 2016-08-19 13:30:36 PDT
Created attachment 286472 [details]
Patch

In the code we use terms:
* Control Flow Profiler
* Code Coverage Profiler
* Basic Block Annotator
* Unexecuted Code Highlights

I'm open to unify some of them.
Comment 6 Brian Burg 2016-08-25 16:21:23 PDT
Comment on attachment 286472 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=286472&action=review

> Source/WebInspectorUI/ChangeLog:35
> +        Make Code Coverage profiler indepedent from Type Profiler.

Nit: independent

> Source/WebInspectorUI/UserInterface/Views/ScriptContentView.js:68
> +        var toolTipControlFlow = WebInspector.UIString("Gray out unexecuted code");

Nit: use 'let'

I don't like this wording. How about "Fade unexecuted code"?

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:178
> +        return !!this._basicBlockAnnotator;

This is super indirect as this is set far away in _makeBasicBlockAnnotator where it's guarded by if (RuntimeAgent.getBasicBlocks) {
If we are keying off this RuntimeAgent method, it would be better to do that here and rewrite the other guard to call this method instead.

Second, this doesn't seem to take into account the value of this.enableControlFlowProfilerSetting. Is this intentional? Some text to explain the different configurations that are now possible would make this a lot easier to review. It is aggravating that this patch introduces more uses of the term "control flow profiler" which so far has been limited to the RuntimeAgent method. I think we should stick to BasicBlockAnnotator-like method and setting names.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:356
> +                    this._setBasicBlockAnnotatorEnabledState(false);

It would be nice to fold the existence checks into _setBasicBlockAnnotatorEnabledState as early exits. It would make this code easier to follow.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1737
>              console.assert(this.visible, "Annotators should not be enabled if the TextEditor is not visible");

This patch doesn't apply anymore so I can't see what this is doing. Can you rebase?

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1761
> +    _setBasicBlockAnnotatorEnabledState(shouldActivate)

This should be a setter. Or I can't figure out why it shouldn't be.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1877
> +    _makeControlFlowScrollEventHandler()

We use 'create' as the prefix for factory methods.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1879
> +        var timeoutIdentifier = null;

Nit: let

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1881
> +        {

Don't put this on its own line.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1885
> +                if (this._basicBlockAnnotator)

Please make an else if {

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1889
> +            timeoutIdentifier = setTimeout(function() {

Use an arrow function instead of .bind(this).

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1896
> +        return scrollHandler.bind(this);

The outer function that's returned should be an arrow function too.

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:162
> +    canShowBasicBlockAnnotator()

This name doesn't make sense. The user doesn't see a BasicBlockAnnotator. They see code coverage/altered styles. I would name it canShowCoverageHints or something.
Comment 7 Nikita Vasilyev 2016-08-25 16:33:52 PDT
Comment on attachment 286472 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=286472&action=review

>> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1761
>> +    _setBasicBlockAnnotatorEnabledState(shouldActivate)
> 
> This should be a setter. Or I can't figure out why it shouldn't be.

This was mirrored from _setTypeTokenAnnotatorEnabledState.
Should it be "_basicBlockAnnotatorEnabled" and "_typeTokenAnnotatorEnabled"?

>> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1877
>> +    _makeControlFlowScrollEventHandler()
> 
> We use 'create' as the prefix for factory methods.

This was mirrored from _makeTypeTokenAnnotator. Should I use "create" for both?
Comment 8 Nikita Vasilyev 2016-08-28 13:09:24 PDT
Created attachment 287235 [details]
Patch
Comment 9 Nikita Vasilyev 2016-08-28 13:18:19 PDT
Created attachment 287236 [details]
Patch
Comment 10 Brian Burg 2016-09-03 14:41:15 PDT
Comment on attachment 287236 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287236&action=review

The patch looks fine overall, but I am worried about the race condition when clicking the button quickly. Can you post a new version with that fixed?

> Source/WebInspectorUI/UserInterface/Base/Main.js:167
> +    this.enableControlFlowProfilerSetting = new WebInspector.Setting("enable-control-flow-profiler", false);

I think this should be on by default. Isn't the performance/deopt impact negligible? We turn it off anyway while profiling.

> Source/WebInspectorUI/UserInterface/Views/ScriptContentView.js:69
> +        let activatedToolTipCodeCoverage = WebInspector.UIString("Donât fade unexecuted code");

Please do not use contractions in UIStrings.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:302
> +            this.updateFormattedState(true).then(() => {

Is it possible for this promise to race with the user clicking on the button again really quickly? What would happen? Maybe toggleUnexecutedCodeHighlights should itself return a promise in all paths, and the button can disable itself until the state update is committed (i.e., the promise resolves).

> Source/WebInspectorUI/UserInterface/Views/TextContentView.js:56
> +        let activatedToolTipCodeCoverage = WebInspector.UIString("Donât fade unexecuted code");

Same comment as above.
Comment 11 Nikita Vasilyev 2016-09-08 14:15:06 PDT
Comment on attachment 287236 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287236&action=review

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:142
> +        return this._codeMirror.historySize().undo > 0;

Pretty printing code creates a CodeMirror history item. This getter always returns `true` for pretty-printed code.
Comment 12 Nikita Vasilyev 2016-09-08 15:19:06 PDT
(In reply to comment #10)
> Comment on attachment 287236 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287236&action=review
> 
> The patch looks fine overall, but I am worried about the race condition when
> clicking the button quickly. Can you post a new version with that fixed?
> 
> > Source/WebInspectorUI/UserInterface/Base/Main.js:167
> > +    this.enableControlFlowProfilerSetting = new WebInspector.Setting("enable-control-flow-profiler", false);
> 
> I think this should be on by default. Isn't the performance/deopt impact
> negligible? We turn it off anyway while profiling.

Saam and I need to measure performance impact first. I think it's better to keep it disabled by default for now.

> 
> > Source/WebInspectorUI/UserInterface/Views/ScriptContentView.js:69
> > +        let activatedToolTipCodeCoverage = WebInspector.UIString("Donât fade unexecuted code");
> 
> Please do not use contractions in UIStrings.
> 
> > Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:302
> > +            this.updateFormattedState(true).then(() => {
> 
> Is it possible for this promise to race with the user clicking on the button
> again really quickly? What would happen? Maybe
> toggleUnexecutedCodeHighlights should itself return a promise in all paths,
> and the button can disable itself until the state update is committed (i.e.,
> the promise resolves).

It is practically impossible to have a race condition here. A minified resource always gets
pretty printed on first open. `this.updateFormattedState`, although asynchronous, is very fast
on consequent calls.

I can disable the button to be make the code less race condition prone, if you want.
Comment 13 Nikita Vasilyev 2016-09-08 15:38:46 PDT
Created attachment 288340 [details]
Patch
Comment 14 WebKit Commit Bot 2016-09-08 15:40:31 PDT
Comment on attachment 288340 [details]
Patch

Rejecting attachment 288340 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 288340, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
file without the binary data in line: "Binary files a/Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js and b/Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js differ".  Be sure to use the --binary flag when invoking "git diff" with diffs containing binary files. at /Volumes/Data/EWS/WebKit/Tools/Scripts/VCSUtils.pm line 820, <ARGV> line 80.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 25 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/2037042
Comment 15 Nikita Vasilyev 2016-09-08 15:42:43 PDT
Created attachment 288342 [details]
Patch
Comment 16 WebKit Commit Bot 2016-09-08 16:15:59 PDT
Comment on attachment 288342 [details]
Patch

Clearing flags on attachment: 288342

Committed r205674: <http://trac.webkit.org/changeset/205674>
Comment 17 WebKit Commit Bot 2016-09-08 16:16:04 PDT
All reviewed patches have been landed.  Closing bug.