Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2371133003:
[Devtools] Lazily build reverse mappings (Closed)
|
Created:
4 years, 2 months ago by eostroukhov Modified:
4 years, 2 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Devtools] Parse source maps lazyly
On some webpages, devtools may run out of memory simply due to the size of the source maps.
BUG=631389
R=lushnikov@chromium.org
Committed: https://crrev.com/70c356fe51101f2ba96769beaf659b7c7c7ad353
Cr-Commit-Position: refs/heads/master@{#422452}
Patch Set 1 #Patch Set 2 : [Devtools] Lazily build reverse mappings #Patch Set 3 : Make all parsing lazy. #
Total comments: 17
Patch Set 4 : Addressing comments #
Total comments: 6
Patch Set 5 : Comments were addressed #
Total comments: 2
Messages
Total messages: 36 (23 generated)
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please take a look
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.
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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
thanks! You might also want to update CL description https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js (right): https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js (right): https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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)?
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [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=lushnikov@chromium.org ========== to ========== [Devtools] Parse source maps lazyly On some webpages, devtools may run out of memory simply due to the size of the source maps. BUG=631389 R=lushnikov@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/Sour... File third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js (right): https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js (right): https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js (right): https://codereview.chromium.org/2371133003/diff/40001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js (right): https://codereview.chromium.org/2371133003/diff/60001/third_party/WebKit/Sour... 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/Sour... 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/Sour... 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]; ...
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the review. I implemented the comments, please take another look. https://codereview.chromium.org/2371133003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/SourceMap.js (right): https://codereview.chromium.org/2371133003/diff/60001/third_party/WebKit/Sour... 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/Sour... 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/Sour... 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lushnikov@chromium.org changed reviewers: + kozyatinskiy@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/Sour... File third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js (right): https://codereview.chromium.org/2371133003/diff/80001/third_party/WebKit/Sour... 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/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/bindings/BlackboxManager.js (right): https://codereview.chromium.org/2371133003/diff/80001/third_party/WebKit/Sour... 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.
The CQ bit was checked by eostroukhov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
[Devtools] Parse source maps lazyly
On some webpages, devtools may run out of memory simply due to the size of the
source maps.
BUG=631389
R=lushnikov@chromium.org
==========
to
==========
[Devtools] Parse source maps lazyly
On some webpages, devtools may run out of memory simply due to the size of the
source maps.
BUG=631389
R=lushnikov@chromium.org
==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from
==========
[Devtools] Parse source maps lazyly
On some webpages, devtools may run out of memory simply due to the size of the
source maps.
BUG=631389
R=lushnikov@chromium.org
==========
to
==========
[Devtools] Parse source maps lazyly
On some webpages, devtools may run out of memory simply due to the size of the
source maps.
BUG=631389
R=lushnikov@chromium.org
Committed: https://crrev.com/70c356fe51101f2ba96769beaf659b7c7c7ad353
Cr-Commit-Position: refs/heads/master@{#422452}
==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/70c356fe51101f2ba96769beaf659b7c7c7ad353 Cr-Commit-Position: refs/heads/master@{#422452} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
