Bug 107238 - Web Inspector: speedup highlight regex API in DefaultTextEditor
Summary: Web Inspector: speedup highlight regex API in DefaultTextEditor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andrey Lushnikov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-18 00:57 PST by Andrey Lushnikov
Modified: 2013-01-23 11:11 PST (History)
9 users (show)

See Also:


Attachments
Patch (15.19 KB, patch)
2013-01-18 01:15 PST, Andrey Lushnikov
no flags Details | Formatted Diff | Diff
Patch (14.89 KB, patch)
2013-01-18 05:44 PST, Andrey Lushnikov
no flags Details | Formatted Diff | Diff
Patch (12.86 KB, patch)
2013-01-23 05:58 PST, Andrey Lushnikov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Lushnikov 2013-01-18 00:57:43 PST
Speedup highlight regex API in DefaultTextEditor. 
Current implementation of highlight regex API forces too many synchronous relayouts of page, which are expensive.
Comment 1 Andrey Lushnikov 2013-01-18 01:15:24 PST
Created attachment 183398 [details]
Patch
Comment 2 Pavel Feldman 2013-01-18 02:30:09 PST
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.
Comment 3 Andrey Lushnikov 2013-01-18 05:44:53 PST
Created attachment 183431 [details]
Patch
Comment 4 Andrey Lushnikov 2013-01-18 05:47:03 PST
Will refactor lineChunks -> lineNumberRanges in a separate patch (to avoid clutter).
Comment 5 Pavel Feldman 2013-01-20 23:33:22 PST
You should reply to me review comments with responses on what you fixed and what not.
Comment 6 Andrey Lushnikov 2013-01-21 01:03:33 PST
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.
Comment 7 Pavel Feldman 2013-01-21 01:56:02 PST
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?
Comment 8 Andrey Lushnikov 2013-01-21 02:42:40 PST
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.
Comment 9 Pavel Feldman 2013-01-21 03:26:15 PST
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.
Comment 10 Andrey Lushnikov 2013-01-21 05:41:57 PST
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?
Comment 11 Pavel Feldman 2013-01-21 05:47:33 PST
> 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.
Comment 12 Pavel Feldman 2013-01-21 05:48:04 PST
> It just feels like the introduced complexity is not worth the feature.

... or better the other way around
Comment 13 Andrey Lushnikov 2013-01-23 05:58:53 PST
Created attachment 184211 [details]
Patch
Comment 14 WebKit Review Bot 2013-01-23 11:11:32 PST
Comment on attachment 184211 [details]
Patch

Clearing flags on attachment: 184211

Committed r140556: <http://trac.webkit.org/changeset/140556>
Comment 15 WebKit Review Bot 2013-01-23 11:11:36 PST
All reviewed patches have been landed.  Closing bug.