Bug 138454 – Web Inspector: Provide a front end for JSC's Control Flow Profiler

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Saam Barati]

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).

[Radar WebKit Bug Importer]

<rdar://problem/18891513>

[Saam Barati]

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.

[Saam Barati]

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.

[Saam Barati]

Created attachment 242729 [details]
work in progress

Mostly the same patch as above, but handle enabling/disabling the TypeTokenAnnotator/BasicBlockAnnotator differently.

[Brian Burg]

*** Bug 42739 has been marked as a duplicate of this bug. ***

[Brian Burg]

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

[Saam Barati]

(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.

[Saam Barati]

Created attachment 244469 [details]
work in progress

[Saam Barati]

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.

[Timothy Hatcher]

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.

[Saam Barati]

(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.

[Timothy Hatcher]

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().

[Saam Barati]

Created attachment 244794 [details]
patch

Took into account Tim's recommendations.

[Saam Barati]

(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

[Timothy Hatcher]

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.

[Saam Barati]

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).

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/178695