Skip to content

Commit 2f333b6

Browse files
legendecasmarco-ippolito
authored andcommitted
lib: optimize prepareStackTrace on builtin frames
Only invalidates source map lookup cache when a new source map is found. This improves when user codes interleave with builtin functions, like `array.map`. PR-URL: #56299 Backport-PR-URL: #59653 Refs: #56296 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Xuguang Mei <meixuguang@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Pietro Marchini <pietro.marchini94@gmail.com>
1 parent cdf9850 commit 2f333b6

File tree

4 files changed

+56
-30
lines changed

4 files changed

+56
-30
lines changed

benchmark/fixtures/simple-error-stack.js

Lines changed: 11 additions & 10 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

benchmark/fixtures/simple-error-stack.ts

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,13 @@
55
const lorem = 'Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.';
66

77
function simpleErrorStack() {
8-
try {
9-
(lorem as any).BANG();
10-
} catch (e) {
11-
return e.stack;
12-
}
8+
[1].map(() => {
9+
try {
10+
(lorem as any).BANG();
11+
} catch (e) {
12+
return e.stack;
13+
}
14+
})
1315
}
1416

1517
export {

lib/internal/source_map/prepare_stack_trace.js

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,9 +53,15 @@ function prepareStackTraceWithSourceMaps(error, trace) {
5353
const sm = fileName === lastFileName ?
5454
lastSourceMap :
5555
findSourceMap(fileName);
56-
lastSourceMap = sm;
57-
lastFileName = fileName;
56+
// Only when a source map is found, cache it for the next iteration.
57+
// This is a performance optimization to avoid interleaving with JS builtin function
58+
// invalidating the cache.
59+
// - at myFunc (file:///path/to/file.js:1:2)
60+
// - at Array.map (<anonymous>)
61+
// - at myFunc (file:///path/to/file.js:3:4)
5862
if (sm) {
63+
lastSourceMap = sm;
64+
lastFileName = fileName;
5965
return `${kStackLineAt}${serializeJSStackFrame(sm, callSite, trace[i + 1])}`;
6066
}
6167
} catch (err) {

lib/internal/source_map/source_map_cache.js

Lines changed: 30 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,9 @@ function maybeCacheSourceMap(filename, content, moduleInstance, isGeneratedSourc
157157
}
158158

159159
const data = dataFromUrl(filename, sourceMapURL);
160+
// `data` could be null if the source map is invalid.
161+
// In this case, create a cache entry with null data with source url for test coverage.
162+
160163
const entry = {
161164
__proto__: null,
162165
lineLengths: lineLengths(content),
@@ -279,6 +282,8 @@ function sourceMapFromDataUrl(sourceURL, url) {
279282
const parsedData = JSONParse(decodedData);
280283
return sourcesToAbsolute(sourceURL, parsedData);
281284
} catch (err) {
285+
// TODO(legendecas): warn about invalid source map JSON string.
286+
// But it could be verbose.
282287
debug(err);
283288
return null;
284289
}
@@ -333,26 +338,38 @@ function sourceMapCacheToObject() {
333338

334339
/**
335340
* Find a source map for a given actual source URL or path.
341+
*
342+
* This function may be invoked from user code or test runner, this must not throw
343+
* any exceptions.
336344
* @param {string} sourceURL - actual source URL or path
337345
* @returns {import('internal/source_map/source_map').SourceMap | undefined} a source map or undefined if not found
338346
*/
339347
function findSourceMap(sourceURL) {
340-
if (RegExpPrototypeExec(kLeadingProtocol, sourceURL) === null) {
341-
sourceURL = pathToFileURL(sourceURL).href;
342-
}
343-
if (!SourceMap) {
344-
SourceMap = require('internal/source_map/source_map').SourceMap;
345-
}
346-
const entry = getModuleSourceMapCache().get(sourceURL) ?? generatedSourceMapCache.get(sourceURL);
347-
if (entry === undefined) {
348+
if (typeof sourceURL !== 'string') {
348349
return undefined;
349350
}
350-
let sourceMap = entry.sourceMap;
351-
if (sourceMap === undefined) {
352-
sourceMap = new SourceMap(entry.data, { lineLengths: entry.lineLengths });
353-
entry.sourceMap = sourceMap;
351+
352+
SourceMap ??= require('internal/source_map/source_map').SourceMap;
353+
try {
354+
if (RegExpPrototypeExec(kLeadingProtocol, sourceURL) === null) {
355+
// If the sourceURL is an invalid path, this will throw an error.
356+
sourceURL = pathToFileURL(sourceURL).href;
357+
}
358+
const entry = getModuleSourceMapCache().get(sourceURL) ?? generatedSourceMapCache.get(sourceURL);
359+
if (entry?.data == null) {
360+
return undefined;
361+
}
362+
363+
let sourceMap = entry.sourceMap;
364+
if (sourceMap === undefined) {
365+
sourceMap = new SourceMap(entry.data, { lineLengths: entry.lineLengths });
366+
entry.sourceMap = sourceMap;
367+
}
368+
return sourceMap;
369+
} catch (err) {
370+
debug(err);
371+
return undefined;
354372
}
355-
return sourceMap;
356373
}
357374

358375
module.exports = {

0 commit comments

Comments
 (0)