Bug 107238 – Web Inspector: speedup highlight regex API in DefaultTextEditor

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Andrey Lushnikov]

Speedup highlight regex API in DefaultTextEditor. 
Current implementation of highlight regex API forces too many synchronous relayouts of page, which are expensive.

[Andrey Lushnikov]

Created attachment 183398 [details]
Patch

[Pavel Feldman]

Comment on attachment 183398 [details]
Patch

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

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1932
> +    _measureOverlayHighlight: function(lineChunks)

lineChunks is a poor name.

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1934
> +        /** @type {Array.<WebInspector.TextEditorMainPanel.RegexOverlayHighlight>} */

nuke this

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1947
> +     * @return {Array.<WebInspector.TextEditorMainPanel.LineOverlayHighlightMetrics>}

Lets use map here.

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1953
> +        for (var i = 0; i < lineChunks.length; ++i) {

this.beginDomUpdates()

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1964
> +                    this._renderRanges(lineRow, line, ranges);

We could do this in an artificial container positioned at 0, 0

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1970
> +        /** @type {Array.<WebInspector.TextEditorMainPanel.LineOverlayHighlightMetrics>} */

This is inferred

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1975
> +            /** @type {WebInspector.TextEditorMainPanel.LineOverlayHighlightMetrics} */

This is implied

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:3058
> +    this.offsetLeft = element.offsetLeft;

You should not use DOM metrics, use simple box metrics that fit your problem: width and offset.

[Andrey Lushnikov]

Created attachment 183431 [details]
Patch

[Andrey Lushnikov]

Will refactor lineChunks -> lineNumberRanges in a separate patch (to avoid clutter).

[Pavel Feldman]

You should reply to me review comments with responses on what you fixed and what not.

[Andrey Lushnikov]

Comment on attachment 183398 [details]
Patch

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

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1932
>> +    _measureOverlayHighlight: function(lineChunks)
> 
> lineChunks is a poor name.

this refactoring is done in a separate patch to avoid clutter: https://bugs.webkit.org/show_bug.cgi?id=107273

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1934
>> +        /** @type {Array.<WebInspector.TextEditorMainPanel.RegexOverlayHighlight>} */
> 
> nuke this

fixed.

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1947
>> +     * @return {Array.<WebInspector.TextEditorMainPanel.LineOverlayHighlightMetrics>}
> 
> Lets use map here.

fixed.

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1953
>> +        for (var i = 0; i < lineChunks.length; ++i) {
> 
> this.beginDomUpdates()

fixed.

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1964
>> +                    this._renderRanges(lineRow, line, ranges);
> 
> We could do this in an artificial container positioned at 0, 0

I did an experiment on that, and it ran significantly slower then current implementation.

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1970
>> +        /** @type {Array.<WebInspector.TextEditorMainPanel.LineOverlayHighlightMetrics>} */
> 
> This is inferred

removed.

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1975
>> +            /** @type {WebInspector.TextEditorMainPanel.LineOverlayHighlightMetrics} */
> 
> This is implied

removed.

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:3058
>> +    this.offsetLeft = element.offsetLeft;
> 
> You should not use DOM metrics, use simple box metrics that fit your problem: width and offset.

fixed.

[Pavel Feldman]

Comment on attachment 183431 [details]
Patch

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

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1929
> +     * @param {Array.<{startLine: number, endLine: number}>} lineChunks

It probably requires a rebaseline?

[Andrey Lushnikov]

Comment on attachment 183431 [details]
Patch

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

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1929
>> +     * @param {Array.<{startLine: number, endLine: number}>} lineChunks
> 
> It probably requires a rebaseline?

The patch (https://bugs.webkit.org/show_bug.cgi?id=107273) was intended to be landed after this.

[Pavel Feldman]

Comment on attachment 183431 [details]
Patch

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

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1896
>          for (var i = 0; i < lineChunks.length; ++i) {

This nesting iteration looks ugly. You iterate over ranges and then over lines within ranges and you need to do this both in _measureOverlayHighlight and below. You also measure overlayHighlight for lines that will get scheduled. I.e. you will re-measure overlayHighlight for them again later.

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1935
> +        for(var key in this._highlightRegexs) {

What is key? Is it a regexString?

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1936
> +            var value = this._highlightRegexs[key];

What is value? Is it a highlightDescriptor? Naming variables key and value when interacting with a map it not helping.

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1940
> +        return overlayHighlight;

It seems suboptimal that you build an array of maps. I'd use multimap instead (i.e. map from line to array of highlights).

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1953
> +        for (var i = 0; i < lineChunks.length; ++i) {

Here it is, same non-trivial nested again. I would be simpler to collect lineRows first in one place and pass them here.

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:3062
> +WebInspector.TextEditorMainPanel.LineOverlayHighlightMetrics;

The fact that you need to define top-level structures like this means that you could have extracted this logic into a separate class. Declaring an array of elements of a type under a name that does not resemble the  name of the items is confusing. How am I supposed to know that LineOverlayHighlightMetrics is an array of ElementMetrics? It should be ElementMetricsArray I guess. And then it just does not need a name.

[Andrey Lushnikov]

Comment on attachment 183431 [details]
Patch

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

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1940
>> +        return overlayHighlight;
> 
> It seems suboptimal that you build an array of maps. I'd use multimap instead (i.e. map from line to array of highlights).

I'll have to store a cssClass for each of the values of a multimap - a slight overhead in comparison to the current version. Will we trade it for convenience?

[Pavel Feldman]

> I'll have to store a cssClass for each of the values of a multimap - a slight overhead in comparison to the current version. Will we trade it for convenience?

It just feels like the introduced complexity is not worth the feature.

[Pavel Feldman]

> It just feels like the introduced complexity is not worth the feature.

... or better the other way around

[Andrey Lushnikov]

Created attachment 184211 [details]
Patch

[WebKit Review Bot]

Comment on attachment 184211 [details]
Patch

Clearing flags on attachment: 184211

Committed r140556: <http://trac.webkit.org/changeset/140556>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.