Summary: | Web Inspector: speedup highlight regex API in DefaultTextEditor | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andrey Lushnikov <lushnikov> | ||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Andrey Lushnikov <lushnikov> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | apavlov, keishi, loislo, pfeldman, pmuellr, vsevik, web-inspector-bugs, webkit.review.bot, yurys | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Andrey Lushnikov
2013-01-18 00:57:43 PST
Created attachment 183398 [details]
Patch
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. Created attachment 183431 [details]
Patch
Will refactor lineChunks -> lineNumberRanges in a separate patch (to avoid clutter). You should reply to me review comments with responses on what you fixed and what not. 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 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 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 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 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? > 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.
> It just feels like the introduced complexity is not worth the feature.
... or better the other way around
Created attachment 184211 [details]
Patch
Comment on attachment 184211 [details] Patch Clearing flags on attachment: 184211 Committed r140556: <http://trac.webkit.org/changeset/140556> All reviewed patches have been landed. Closing bug. |