[Devtools] Lazily build reverse mappings (issue 2371133003 by eostroukhov@chromium.org)

[eostr...@chromium.org]

Reviewers: lushnikov
CL: https://codereview.chromium.org/2371133003/

Message:
Please take a look

Description:
[Devtools] Lazily build reverse mappings

Reverse mappings are large enough to cause crashes in at least on case.
This code will lazily compute them to save some memory. On my system,
building reverse mapping for a single source URL takes 25 ms for source
map with 500000 entries. Currently this delay is most noticeable when
the file is being opened in an editor.

Note that these lazily constructed reverse mappings are currently never
cleaned up. I'd like to see if any user will be able to hit the problem
before adding more complexity.

BUG=631389
R=lush...@chromium.org

Affected files (+18, -33 lines):
M third_party/WebKit/LayoutTests/http/tests/inspector/compiler-script-mapping.html
M third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js
M third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js
M third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js


Index: third_party/WebKit/LayoutTests/http/tests/inspector/compiler-script-mapping.html
diff --git a/third_party/WebKit/LayoutTests/http/tests/inspector/compiler-script-mapping.html b/third_party/WebKit/LayoutTests/http/tests/inspector/compiler-script-mapping.html
index 528c6afab6237c784a507e471d1dfd01e07c66a8..c7c3122ad6107928f9177b9b1b4b9e15349e73f3 100644
--- a/third_party/WebKit/LayoutTests/http/tests/inspector/compiler-script-mapping.html
+++ b/third_party/WebKit/LayoutTests/http/tests/inspector/compiler-script-mapping.html
@@ -150,7 +150,7 @@ function test()
}
]};
var mapping = new WebInspector.TextSourceMap("compiled.js", "source-map.json", mappingPayload);
- InspectorTest.assertEquals(2, mapping.sourceURLs().length);
+ InspectorTest.assertEquals(2, Array.from(mapping.sourceURLs()).length);
checkMapping(0, 0, "source1.js", 0, 0, mapping);
checkMapping(0, 1, "source1.js", 2, 1, mapping);
checkMapping(2, 10, "source2.js", 0, 0, mapping);
Index: third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js
diff --git a/third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js b/third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js
index ca159e812afcdc40a2598a1637181ff33b461836..63a91bdb4a7b42df492cd6dd80f55b6ebc471f13 100644
--- a/third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js
+++ b/third_party/WebKit/Source/devtools/front_end/bindings/CompilerScriptMapping.js
@@ -232,10 +232,8 @@ WebInspector.CompilerScriptMapping.prototype = {
this._scriptForSourceMap.set(sourceMap, script);

// Report sources.
- var sourceURLs = sourceMap.sourceURLs();
var missingSources = [];
- for (var i = 0; i < sourceURLs.length; ++i) {
- var sourceURL = sourceURLs[i];
+ for (const sourceURL of sourceMap.sourceURLs()) {
if (this._sourceMapForURL.get(sourceURL))
continue;
this._sourceMapForURL.set(sourceURL, sourceMap);
@@ -370,9 +368,8 @@ WebInspector.CompilerScriptMapping.prototype = {
var script = this._scriptForSourceMap.get(sourceMap);
if (!script)
return;
- var sourceURLs = sourceMap.sourceURLs();
- for (var i = 0; i < sourceURLs.length; ++i) {
- var uiSourceCode = this._networkMapping.uiSourceCodeForScriptURL(sourceURLs[i], script);
+ for (const sourceURL of sourceMap.sourceURLs()) {
+ var uiSourceCode = this._networkMapping.uiSourceCodeForScriptURL(sourceURL, script);
if (uiSourceCode)
this._unbindUISourceCode(uiSourceCode);
}
Index: third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js
diff --git a/third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js b/third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js
index 0a10c72cb5445a7852105235bdf23cceb0f928ca..40e7bb6e5f29264b80150dac7a6aedd7c1ff9194 100644
--- a/third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js
+++ b/third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js
@@ -212,7 +212,9 @@ WebInspector.CSSModel.prototype = {
*/
_factoryForSourceMap: function(sourceMap)
{
- var sourceExtensions = new Set(sourceMap.sourceURLs().map(url => WebInspector.ParsedURL.extractExtension(url)));
+ var sourceExtensions = new Set();
+ for (const url of sourceMap.sourceURLs())
+ sourceExtensions.add(WebInspector.ParsedURL.extractExtension(url));
for (var runtimeExtension of self.runtime.extensions(WebInspector.SourceMapFactory)) {
var supportedExtensions = new Set(runtimeExtension.descriptor()["extensions"]);
if (supportedExtensions.containsAll(sourceExtensions))
Index: third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js
diff --git a/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js b/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js
index 58c6def49e424619adb62bfab9bcaacf1a62423e..5d7209a579b19af44542e42e4e7c5eed547e1377 100644
--- a/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js
+++ b/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js
@@ -96,7 +96,7 @@ WebInspector.SourceMap.prototype = {
url: function() { },

/**
- * @return {!Array<string>}
+ * @return {!Iterable<string>|!Array<string>}
*/
sourceURLs: function() { },

@@ -174,9 +174,8 @@ WebInspector.TextSourceMap = function(compiledURL, sourceMappingURL, payload)

this._compiledURL = compiledURL;
this._sourceMappingURL = sourceMappingURL;
- this._reverseMappingsBySourceURL = new Map();
this._mappings = [];
- this._sources = {};
+ this._sources = new Map();
this._sourceContentByURL = {};
this._parseMappingPayload(payload);
}
@@ -242,11 +241,11 @@ WebInspector.TextSourceMap.prototype = {

/**
* @override
- * @return {!Array.<string>}
+ * @return {!Iterable.<string>}
*/
sourceURLs: function()
{
- return Object.keys(this._sources);
+ return this._sources.keys();
},

/**
@@ -299,10 +298,8 @@ WebInspector.TextSourceMap.prototype = {
*/
_parseSections: function(sections)
{
- for (var i = 0; i < sections.length; ++i) {
- var section = sections[i];
+ for (const section of sections)
this._parseMap(section.map, section.offset.line, section.offset.column);
- }
},

/**
@@ -370,12 +367,12 @@ WebInspector.TextSourceMap.prototype = {
*/
_reversedMappings: function(sourceURL)
{
- var mappings = this._reverseMappingsBySourceURL.get(sourceURL);
- if (!mappings)
+ if (!this._sources.has(sourceURL))
return [];
- if (!mappings._sorted) {
- mappings.sort(sourceMappingComparator);
- mappings._sorted = true;
+ let mappings = this._sources.get(sourceURL);
+ if (mappings === null) {
+ mappings = this._mappings.filter((mapping) => mapping.sourceURL === sourceURL).sort(sourceMappingComparator);
+ this._sources.set(sourceURL, mappings)
}
return mappings;

@@ -422,7 +419,7 @@ WebInspector.TextSourceMap.prototype = {
if (url === this._compiledURL && hasSource)
url += WebInspector.UIString(" [sm]");
sources.push(url);
- this._sources[url] = true;
+ this._sources.set(url, null);

if (hasSource)
this._sourceContentByURL[url] = map.sourcesContent[i];
@@ -466,17 +463,6 @@ WebInspector.TextSourceMap.prototype = {
nameIndex += this._decodeVLQ(stringCharIterator);
this._mappings.push(new WebInspector.SourceMapEntry(lineNumber, columnNumber, sourceURL, sourceLineNumber, sourceColumnNumber, names[nameIndex]));
}
-
- for (var i = 0; i < this._mappings.length; ++i) {
- var mapping = this._mappings[i];
- var url = mapping.sourceURL;
- if (!url)
- continue;
- if (!this._reverseMappingsBySourceURL.has(url))
- this._reverseMappingsBySourceURL.set(url, []);
- var reverseMappings = this._reverseMappingsBySourceURL.get(url);
- reverseMappings.push(mapping);
- }
},

/**

[pfel...@chromium.org]

How does making it lazy address the memory issue? Also, why does it take much
memory? It seems like we should figure out the root cause.

https://codereview.chromium.org/2371133003/

[eostr...@chromium.org]

On 2016/09/27 18:42:39, pfeldman wrote:
> How does making it lazy address the memory issue? Also, why does it take much
> memory? It seems like we should figure out the root cause.

The difference is that the user does not need all reverse mappings for all
source URLs. As I mentioned in the commit comment, we may want to eventually
have a more comprehensive (and complex) solution - but I believe this one should
do for now.

One solution we've considered was to parse the source maps lazily, we might want
to do that at some later point.

https://codereview.chromium.org/2371133003/

[eostr...@chromium.org]

Please take another look.

I also have an idea for more compact storage of the parsed entries. I am not
sure we need it right now, but it may be worth considering.

https://codereview.chromium.org/2371133003/

[lush...@chromium.org]

thanks! You might also want to update CL description


https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js
File
third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js
(right):

https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js#newcode154
third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js:154:
var hasBlackboxedMappings = false;
what's this code about?

https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js
File third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js
(right):

https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js#newcode99
third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js:99: *

@return {!Iterable<string>|!Array<string>}

let's keep this array - we try to avoid polymorphic types as hard as
possible

https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js#newcode180
third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js:180:
this._sources = new Map();
1. let's add jsdoc
2. let's name this _sourceURLToReversedMappings

https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js#newcode249
third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js:249:
return this._sources.keys();
let's use .keysArray() to return array

https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js#newcode359
third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js:359: let
mappings = this.mappings();
nit: we're not yet using "let" in the codebase

https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js#newcode390
third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js:390: const
sections = this._json.sections || [this._json];
let's bail out early instead:

if (!this._json.sections) {
callback(this._json, 0, 0);
return;
}

...

https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js#newcode391
third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js:391: for
(const section of sections) {
nit: we're not yet using "const" in the codebase

https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js#newcode404
third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js:404: for
(var i = 0; i < sourceMap.sources.length; ++i) {
this looks like a copy of the code in _parseMap. Can we either extract
this into a separate function, or maybe re-use sources for the section
(so that we don't need to re-do this in _parseMap)?

https://codereview.chromium.org/2371133003/

[eostr...@chromium.org]

Thank you for the review. I addressed the comments and fixed the failing test,
please take another look.

https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js
File
third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js
(right):

https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js#newcode154
third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js:154:
var hasBlackboxedMappings = false;

On 2016/09/28 16:48:12, lushnikov wrote:
> what's this code about?

There is no reason to check individual mappings if no source URLs from
the source map are blackboxed.

I rewrote the code a bit to make the change smaller.

https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js
File third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js
(right):

https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js#newcode99
third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js:99: *
@return {!Iterable<string>|!Array<string>}

On 2016/09/28 16:48:12, lushnikov wrote:
> let's keep this array - we try to avoid polymorphic types as hard as
possible

This CL is about saving memory! ;)

(Ok, I fixed this)

https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js#newcode180
third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js:180:
this._sources = new Map();

On 2016/09/28 16:48:12, lushnikov wrote:
> 1. let's add jsdoc
> 2. let's name this _sourceURLToReversedMappings

Done.

https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js#newcode249
third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js:249:
return this._sources.keys();

On 2016/09/28 16:48:13, lushnikov wrote:
> let's use .keysArray() to return array

Done.

https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js#newcode359
third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js:359: let
mappings = this.mappings();

On 2016/09/28 16:48:13, lushnikov wrote:
> nit: we're not yet using "let" in the codebase

Done.

https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js#newcode390
third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js:390: const
sections = this._json.sections || [this._json];

On 2016/09/28 16:48:13, lushnikov wrote:
> let's bail out early instead:
>
> if (!this._json.sections) {
> callback(this._json, 0, 0);
> return;
> }
>
> ...

Done.

https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js#newcode391
third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js:391: for
(const section of sections) {

On 2016/09/28 16:48:13, lushnikov wrote:
> nit: we're not yet using "const" in the codebase

Done.

https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js#newcode404
third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js:404: for
(var i = 0; i < sourceMap.sources.length; ++i) {

On 2016/09/28 16:48:13, lushnikov wrote:
> this looks like a copy of the code in _parseMap. Can we either extract
this into
> a separate function, or maybe re-use sources for the section (so that
we don't
> need to re-do this in _parseMap)?

There are building different things - one is building source-map wide
collection of the sourceURLs and another needs a per-section array of
the same URLs. I tried to refactor it, ended up with another callback.

https://codereview.chromium.org/2371133003/

[lush...@chromium.org]

https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js
File
third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js
(right):

https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js#newcode154
third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js:154:
var hasBlackboxedMappings = false;

On 2016/09/28 23:26:33, eostroukhov wrote:
> On 2016/09/28 16:48:12, lushnikov wrote:
> > what's this code about?
>
> There is no reason to check individual mappings if no source URLs from
the
> source map are blackboxed.
>
> I rewrote the code a bit to make the change smaller.

Nice, thanks

https://codereview.chromium.org/2371133003/diff/60001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js
File third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js
(right):

https://codereview.chromium.org/2371133003/diff/60001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js#newcode87
third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js:87:
WebInspector.SourceURLInfo = function(content, reverseMappings) {
WebInspector.SourceMap.SourceInfo?

https://codereview.chromium.org/2371133003/diff/60001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js#newcode367
third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js:367: if
(this._sourceURLInfos && !this._sourceURLInfos.has(sourceURL))
how could this._sourceURLInfos be null?

https://codereview.chromium.org/2371133003/diff/60001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js#newcode427
third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js:427:
_parseSources: function(sourceMap) {
yeah, callback is unfortunate

Let's do the following:

_parseSources: function(sourceMap)
{
var sources = getSourcesForSourceMap(sourceMap);
this._sourceURLInfos.addAll(sources);
sourceMap[sourcesSymbol] = sources;
}

and later, in the _parseMap, we can just get sources for the SourceMap:

...
var sources = sourceMap[sourcesSymbol];
...

https://codereview.chromium.org/2371133003/

[eostr...@chromium.org]

Thanks for the review. I implemented the comments, please take another look.

https://codereview.chromium.org/2371133003/diff/60001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js
File third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js
(right):

https://codereview.chromium.org/2371133003/diff/60001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js#newcode87
third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js:87:
WebInspector.SourceURLInfo = function(content, reverseMappings) {

On 2016/09/29 16:29:52, lushnikov wrote:
> WebInspector.SourceMap.SourceInfo?

Done, made it TextSourceMap implementation detail.

https://codereview.chromium.org/2371133003/diff/60001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js#newcode367
third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js:367: if
(this._sourceURLInfos && !this._sourceURLInfos.has(sourceURL))

On 2016/09/29 16:29:52, lushnikov wrote:
> how could this._sourceURLInfos be null?

It can't. Thanks for pointing out.

https://codereview.chromium.org/2371133003/diff/60001/third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js#newcode427
third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js:427:
_parseSources: function(sourceMap) {

On 2016/09/29 16:29:53, lushnikov wrote:
> yeah, callback is unfortunate
>
> Let's do the following:
>
> _parseSources: function(sourceMap)
> {
> var sources = getSourcesForSourceMap(sourceMap);
> this._sourceURLInfos.addAll(sources);
> sourceMap[sourcesSymbol] = sources;
> }
>
> and later, in the _parseMap, we can just get sources for the
SourceMap:
>
> ...
> var sources = sourceMap[sourcesSymbol];
> ...

Done.

https://codereview.chromium.org/2371133003/

[lush...@chromium.org]

lgtm, thanks!

I'm confused about the BlackboxManager part - adding Alexey to look into it


https://codereview.chromium.org/2371133003/diff/80001/third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js
File
third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js
(right):

https://codereview.chromium.org/2371133003/diff/80001/third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js#newcode154
third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js:154:
if (!mappings.length) {
how's a sourceMap with empty mappings differs from a "no sourcemap"
case?

@kozyatinskiy: could you please give some insights on this?

https://codereview.chromium.org/2371133003/

[kozyat...@chromium.org]

https://codereview.chromium.org/2371133003/diff/80001/third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js
File
third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js
(right):

https://codereview.chromium.org/2371133003/diff/80001/third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js#newcode154
third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js:154:
if (!mappings.length) {

On 2016/10/01 04:23:50, lushnikov wrote:
> how's a sourceMap with empty mappings differs from a "no sourcemap"
case?
>
> @kozyatinskiy: could you please give some insights on this?

There are no big difference between empty source map and no source map.
This cases can be merge into no mappings case.

lgtm for blackboxmanager.

https://codereview.chromium.org/2371133003/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2371133003/80001


https://codereview.chromium.org/2371133003/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #5 (id:80001)

https://codereview.chromium.org/2371133003/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 5 (id:??) landed as
https://crrev.com/70c356fe51101f2ba96769beaf659b7c7c7ad353
Cr-Commit-Position: refs/heads/master@{#422452}

https://codereview.chromium.org/2371133003/