Bug 138454 - Web Inspector: Provide a front end for JSC's Control Flow Profiler
Summary: Web Inspector: Provide a front end for JSC's Control Flow Profiler
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
: 42739 (view as bug list)
Depends on: 140377
Blocks:
  Show dependency treegraph
 
Reported: 2014-11-05 22:33 PST by Saam Barati
Modified: 2015-01-19 21:17 PST (History)
5 users (show)

See Also:


Attachments
work in progress (7.88 KB, patch)
2014-11-07 13:00 PST, Saam Barati
no flags Details | Formatted Diff | Diff
work in progress (29.68 KB, patch)
2014-12-06 16:57 PST, Saam Barati
no flags Details | Formatted Diff | Diff
work in progress (30.38 KB, patch)
2014-12-06 17:20 PST, Saam Barati
no flags Details | Formatted Diff | Diff
work in progress (20.61 KB, patch)
2015-01-12 15:16 PST, Saam Barati
no flags Details | Formatted Diff | Diff
work in progress (26.95 KB, patch)
2015-01-15 17:07 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (34.87 KB, patch)
2015-01-16 13:03 PST, Saam Barati
timothy: review+
Details | Formatted Diff | Diff
patch (36.07 KB, patch)
2015-01-16 23:35 PST, Saam Barati
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2014-11-05 22:33:22 PST
JSC is developing a control flow profiler that will profile which basic blocks have executed.
This patch should provide an API for the Web Inspector to use and also create a UI around this API.
The API will work by sending a list of the following 3-tuples:
(<int> basic block start offset, <int> basic block end offset, <bool> has executed).
Comment 1 Radar WebKit Bug Importer 2014-11-05 22:33:35 PST
<rdar://problem/18891513>
Comment 2 Saam Barati 2014-11-07 13:00:38 PST
Created attachment 241201 [details]
work in progress

This is a simple front end to JSC's backend basic block ranges. It simply grabs the ranges JSC gives it and displays the background as either gray or white.
Comment 3 Saam Barati 2014-12-06 16:57:10 PST
Created attachment 242728 [details]
work in progress

I'm thinking of breaking this up into two patches, one that creates the parent class JavaScriptAnnotator and JavaScriptAnnotatorManager, and another that does this UI work for basic block highlighting.
Comment 4 Saam Barati 2014-12-06 17:20:48 PST
Created attachment 242729 [details]
work in progress

Mostly the same patch as above, but handle enabling/disabling the TypeTokenAnnotator/BasicBlockAnnotator differently.
Comment 5 Brian Burg 2014-12-16 00:29:28 PST
*** Bug 42739 has been marked as a duplicate of this bug. ***
Comment 6 Brian Burg 2014-12-17 10:06:12 PST
Comment on attachment 242729 [details]
work in progress

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

Looking good so far. You might want to split the AnnotatorManager refactor into its own bug if it's not too big a problem. ChangeLog entries will help a lot with understanding some of the API changes.

> Source/WebInspectorUI/UserInterface/Controllers/BasicBlockAnnotator.js:88
> +        // FIXME: Eventually, the back end data shouldn't give us these bad ranges.

Bugzilla number?

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:611
> +        console.log("gained focus");

Oops
Comment 7 Saam Barati 2014-12-17 10:59:22 PST
(In reply to comment #6)
> Comment on attachment 242729 [details]
> work in progress
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=242729&action=review
> 
> Looking good so far. You might want to split the AnnotatorManager refactor
> into its own bug if it's not too big a problem. ChangeLog entries will help
> a lot with understanding some of the API changes.

Agreed. I've split out *Annotator and AnnotatorManager into this bug:
https://bugs.webkit.org/show_bug.cgi?id=139426

Do you think I should have AnnotatorManager in its own patch?

> 
> > Source/WebInspectorUI/UserInterface/Controllers/BasicBlockAnnotator.js:88
> > +        // FIXME: Eventually, the back end data shouldn't give us these bad ranges.
> 
> Bugzilla number?

I should remove this, it's no longer the case that this may happen.
Comment 8 Saam Barati 2015-01-12 15:16:47 PST
Created attachment 244469 [details]
work in progress
Comment 9 Saam Barati 2015-01-15 17:07:44 PST
Created attachment 244732 [details]
work in progress

This patch is pretty close to final. I think the only thing left
is to pause basic block highlighted when an error is encountered.

Any feedback is greatly appreciated.
Comment 10 Timothy Hatcher 2015-01-15 19:06:47 PST
Comment on attachment 244732 [details]
work in progress

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

> Source/WebInspectorUI/UserInterface/Controllers/BasicBlockAnnotator.js:117
> +            var startLineEndPosition = { line: startPosition.line, ch: this.sourceCodeTextEditor.line(startPosition.line).length };

I think if you leave ch off it will be end of line.

> Source/WebInspectorUI/UserInterface/Controllers/BasicBlockAnnotator.js:156
> +    _canGrayOutEntireLine: function(lineNumber, startPosition, endPosition)

This should assert startPosition.line === endPosition.line.

> Source/WebInspectorUI/UserInterface/Controllers/BasicBlockAnnotator.js:163
> +        lineText = lineText.replace(/\s/g, "");
> +        blockText = blockText.replace(/\s/g, "");
> +        if (lineText === blockText)
> +            return true;

Ideally this shouldn't use replace, as that would be heavy if this gets called a lot. I there another approach that uses _isTextRangeOnlyWhitespace?

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:271
> +        var basicBlockAnnotator = this._annotatorManager.getAnnotator(WebInspector.SourceCodeTextEditor.BasicBlockAnnotatorKey);

Not new to this patch: I don't see the point of having AnnotatorManager. It seems like we could/should just have annotators, and use them directly. Requiring a key/map to look up annotators seems heavy when we could just access them as properties on SourceCodeTextEditor directly.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:287
>          var typeTokenAnnotator = this._annotatorManager.getAnnotator(WebInspector.SourceCodeTextEditor.TypeTokenAnnotatorKey);
> +        var basicBlockAnnotator = this._annotatorManager.getAnnotator(WebInspector.SourceCodeTextEditor.BasicBlockAnnotatorKey);

Like this should just be this._typeTokenAnnotator and this._basicBlockAnnotator. Unless there is a deeper reason I don't see.
Comment 11 Saam Barati 2015-01-16 10:21:22 PST
(In reply to comment #10)
> Comment on attachment 244732 [details]
> work in progress
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244732&action=review
> 
> > Source/WebInspectorUI/UserInterface/Controllers/BasicBlockAnnotator.js:117
> > +            var startLineEndPosition = { line: startPosition.line, ch: this.sourceCodeTextEditor.line(startPosition.line).length };
> 
> I think if you leave ch off it will be end of line.
> 
I personally like that it is explicitly written out. I suppose the name of the variable would indicate this if ch is left off, but I can foresee myself reading the LOC with ch left off in the future and forgetting this.

> > Source/WebInspectorUI/UserInterface/Controllers/BasicBlockAnnotator.js:156
> > +    _canGrayOutEntireLine: function(lineNumber, startPosition, endPosition)
> 
> This should assert startPosition.line === endPosition.line.
> 
> > Source/WebInspectorUI/UserInterface/Controllers/BasicBlockAnnotator.js:163
> > +        lineText = lineText.replace(/\s/g, "");
> > +        blockText = blockText.replace(/\s/g, "");
> > +        if (lineText === blockText)
> > +            return true;
> 
> Ideally this shouldn't use replace, as that would be heavy if this gets
> called a lot. I there another approach that uses _isTextRangeOnlyWhitespace?
> 

Yeah, I think this can be done just by:
return lineText.trim() === blockText.trim()

> > Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:271
> > +        var basicBlockAnnotator = this._annotatorManager.getAnnotator(WebInspector.SourceCodeTextEditor.BasicBlockAnnotatorKey);
> 
> Not new to this patch: I don't see the point of having AnnotatorManager. It
> seems like we could/should just have annotators, and use them directly.
> Requiring a key/map to look up annotators seems heavy when we could just
> access them as properties on SourceCodeTextEditor directly.
> 

After writing this patch, I tend to agree. I think it would be nicer just having these as properties on SourceCodeTextEditor. I originally thought that there would be more places where I could just group operations under the Manager, but there are enough exceptions here that it's a bit circuitous to use the Manager. The goal of the Manager was to create an easy way for other *Annotator classes to be created and easily integrated in SourceCodeTextEditor, but I think that it's currently not worth it. I'm going to remove the Manager.

> > Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:287
> >          var typeTokenAnnotator = this._annotatorManager.getAnnotator(WebInspector.SourceCodeTextEditor.TypeTokenAnnotatorKey);
> > +        var basicBlockAnnotator = this._annotatorManager.getAnnotator(WebInspector.SourceCodeTextEditor.BasicBlockAnnotatorKey);
> 
> Like this should just be this._typeTokenAnnotator and
> this._basicBlockAnnotator. Unless there is a deeper reason I don't see.
Comment 12 Timothy Hatcher 2015-01-16 11:21:30 PST
Comment on attachment 244732 [details]
work in progress

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

>>> Source/WebInspectorUI/UserInterface/Controllers/BasicBlockAnnotator.js:156
>>> +    _canGrayOutEntireLine: function(lineNumber, startPosition, endPosition)
>> 
>> This should assert startPosition.line === endPosition.line.
> 
> Yeah, I think this can be done just by:
> return lineText.trim() === blockText.trim()

Using trim() s just as expensive as replace().
Comment 13 Saam Barati 2015-01-16 13:03:04 PST
Created attachment 244794 [details]
patch

Took into account Tim's recommendations.
Comment 14 Saam Barati 2015-01-16 13:29:14 PST
(In reply to comment #12)
> Comment on attachment 244732 [details]
> work in progress
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244732&action=review
> 
> >>> Source/WebInspectorUI/UserInterface/Controllers/BasicBlockAnnotator.js:156
> >>> +    _canGrayOutEntireLine: function(lineNumber, startPosition, endPosition)
> >> 
> >> This should assert startPosition.line === endPosition.line.
> > 
> > Yeah, I think this can be done just by:
> > return lineText.trim() === blockText.trim()
> 
> Using trim() s just as expensive as replace().

As Tim and I discussed on IRC, we can do this and still be performant because the BasicBlockAnnotator will only view basic blocks that are visible on screen, so the number of strings created/compared is proportional to the number of basic blocks on screen and not proportional to the number of basic blocks in the program.

The bigger problem is when the strings are very long. See:
https://bugs.webkit.org/show_bug.cgi?id=140550
Comment 15 Timothy Hatcher 2015-01-16 14:22:24 PST
Comment on attachment 244794 [details]
patch

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

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:356
> +        var shouldResumeTypeTokenAnnotator = this._typeTokenAnnotator && this._typeTokenAnnotator.isActive;

Nit: Not new, but we typically treat things like isActive as a function, not a getter. My rule: if it sounds like a question, it is a function.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1625
> -        var typeTokenAnnotator = new WebInspector.TypeTokenAnnotator(this, script);
> -        this._annotatorManager.addAnnotator(WebInspector.SourceCodeTextEditor.TypeTokenAnnotatorKey, typeTokenAnnotator);
> +        this._typeTokenAnnotator = new WebInspector.TypeTokenAnnotator(this, script);

I like this much better.
Comment 16 Saam Barati 2015-01-16 23:35:04 PST
Created attachment 244838 [details]
patch

This is the same patch as before, but I've included a change
that grays out the white padding on the left side of the TextEditor
when a text range is marked but not the entire line is marked.
(This usually occurs on the last line of a basic block).
Comment 17 Saam Barati 2015-01-19 21:17:27 PST
landed in:
http://trac.webkit.org/changeset/178695