Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

lib,src: add source map support for global eval #43428

Closed
wants to merge 2 commits into from
Closed

lib,src: add source map support for global eval #43428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
775dcb0
lib,src: add source map support for global eval
legendecas Jun 13, 2022
4da000b
fixup! windows path separator
legendecas Jul 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions benchmark/es/eval.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
'use strict';

const common = require('../common.js');

const bench = common.createBenchmark(main, {
method: ['without-sourcemap', 'sourcemap'],
n: [1e6],
});

const sourceWithoutSourceMap = `
'use strict';
(function() {
let a = 1;
for (let i = 0; i < 1000; i++) {
a++;
}
return a;
})();
`;
const sourceWithSourceMap = `
${sourceWithoutSourceMap}
//# sourceMappingURL=https://ci.nodejs.org/405
`;

function evalN(n, source) {
bench.start();
for (let i = 0; i < n; i++) {
eval(source);
}
bench.end(n);
}

function main({ n, method }) {
switch (method) {
case 'without-sourcemap':
process.setSourceMapsEnabled(false);
evalN(n, sourceWithoutSourceMap);
break;
case 'sourcemap':
process.setSourceMapsEnabled(true);
evalN(n, sourceWithSourceMap);
break;
default:
throw new Error(`Unexpected method "${method}"`);
}
}
9 changes: 7 additions & 2 deletions lib/internal/source_map/prepare_stack_trace.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,15 @@ const prepareStackTrace = (globalThis, error, trace) => {
try {
// A stack trace will often have several call sites in a row within the
// same file, cache the source map and file content accordingly:
const fileName = t.getFileName();
let fileName = t.getFileName();
let generated = false;
if (fileName === undefined) {
fileName = t.getEvalOrigin();
bcoe marked this conversation as resolved.
Show resolved Hide resolved
generated = true;
}
const sm = fileName === lastFileName ?
lastSourceMap :
findSourceMap(fileName);
findSourceMap(fileName, generated);
lastSourceMap = sm;
lastFileName = fileName;
if (sm) {
Expand Down
69 changes: 55 additions & 14 deletions lib/internal/source_map/source_map_cache.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,18 @@ const {
normalizeReferrerURL,
} = require('internal/modules/cjs/helpers');
const { validateBoolean } = require('internal/validators');
const { setMaybeCacheGeneratedSourceMap } = internalBinding('errors');

// Since the CJS module cache is mutable, which leads to memory leaks when
// modules are deleted, we use a WeakMap so that the source map cache will
// be purged automatically:
const cjsSourceMapCache = new IterableWeakMap();
// The esm cache is not mutable, so we can use a Map without memory concerns:
const esmSourceMapCache = new SafeMap();
// The generated sources is not mutable, so we can use a Map without memory concerns:
const generatedSourceMapCache = new SafeMap();
const kLeadingProtocol = /^\w+:\/\//;

const { fileURLToPath, pathToFileURL, URL } = require('internal/url');
let SourceMap;

Expand Down Expand Up @@ -71,14 +77,13 @@ function setSourceMapsEnabled(val) {
sourceMapsEnabled = val;
}

function maybeCacheSourceMap(filename, content, cjsModuleInstance) {
function maybeCacheSourceMap(filename, content, cjsModuleInstance, isGeneratedSource) {
const sourceMapsEnabled = getSourceMapsEnabled();
if (!(process.env.NODE_V8_COVERAGE || sourceMapsEnabled)) return;
try {
filename = normalizeReferrerURL(filename);
} catch (err) {
// This is most likely an [eval]-wrapper, which is currently not
// supported.
// This is most likely an invalid filename in sourceURL of [eval]-wrapper.
debug(err);
return;
}
Expand All @@ -96,8 +101,14 @@ function maybeCacheSourceMap(filename, content, cjsModuleInstance) {
data,
url
});
} else if (isGeneratedSource) {
generatedSourceMapCache.set(filename, {
lineLengths: lineLengths(content),
data,
url
});
} else {
// If there is no cjsModuleInstance assume we are in a
// If there is no cjsModuleInstance and is not generated source assume we are in a
// "modules/esm" context.
esmSourceMapCache.set(filename, {
lineLengths: lineLengths(content),
Expand All @@ -108,6 +119,31 @@ function maybeCacheSourceMap(filename, content, cjsModuleInstance) {
}
}

function maybeCacheGeneratedSourceMap(content) {
const sourceMapsEnabled = getSourceMapsEnabled();
if (!(process.env.NODE_V8_COVERAGE || sourceMapsEnabled)) return;

const matchSourceURL = RegExpPrototypeExec(
/\/[*/]#\s+sourceURL=(?<sourceURL>[^\s]+)/,
content
);
if (matchSourceURL == null) {
return;
}
let sourceURL = matchSourceURL.groups.sourceURL;
if (RegExpPrototypeExec(kLeadingProtocol, sourceURL) === null) {
sourceURL = pathToFileURL(sourceURL).href;
}
try {
maybeCacheSourceMap(sourceURL, content, null, true);
} catch (err) {
// This can happen if the filename is not a valid URL.
// If we fail to cache the source map, we should not fail the whole process.
debug(err);
}
}
setMaybeCacheGeneratedSourceMap(maybeCacheGeneratedSourceMap);

function dataFromUrl(sourceURL, sourceMappingURL) {
try {
const url = new URL(sourceMappingURL);
Expand Down Expand Up @@ -218,21 +254,26 @@ function appendCJSCache(obj) {
}
}

function findSourceMap(sourceURL) {
if (RegExpPrototypeExec(/^\w+:\/\//, sourceURL) === null) {
function findSourceMap(sourceURL, isGenerated) {
if (RegExpPrototypeExec(kLeadingProtocol, sourceURL) === null) {
sourceURL = pathToFileURL(sourceURL).href;
}
if (!SourceMap) {
SourceMap = require('internal/source_map/source_map').SourceMap;
}
let sourceMap = esmSourceMapCache.get(sourceURL);
if (sourceMap === undefined) {
for (const value of cjsSourceMapCache) {
const filename = ObjectGetValueSafe(value, 'filename');
if (sourceURL === filename) {
sourceMap = {
data: ObjectGetValueSafe(value, 'data')
};
let sourceMap;
if (isGenerated) {
sourceMap = generatedSourceMapCache.get(sourceURL);
} else {
sourceMap = esmSourceMapCache.get(sourceURL);
if (sourceMap === undefined) {
for (const value of cjsSourceMapCache) {
const filename = ObjectGetValueSafe(value, 'filename');
if (sourceURL === filename) {
sourceMap = {
data: ObjectGetValueSafe(value, 'data')
};
}
}
}
}
Expand Down
5 changes: 5 additions & 0 deletions src/api/environment.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,8 @@ void SetIsolateMiscHandlers(v8::Isolate* isolate, const IsolateSettings& s) {
auto* allow_wasm_codegen_cb = s.allow_wasm_code_generation_callback ?
s.allow_wasm_code_generation_callback : AllowWasmCodeGenerationCallback;
isolate->SetAllowWasmCodeGenerationCallback(allow_wasm_codegen_cb);
isolate->SetModifyCodeGenerationFromStringsCallback(
ModifyCodeGenerationFromStrings);

Mutex::ScopedLock lock(node::per_process::cli_options_mutex);
if (per_process::cli_options->get_per_isolate_options()
Expand Down Expand Up @@ -665,6 +667,9 @@ Maybe<bool> InitializeContextForSnapshot(Local<Context> context) {
Isolate* isolate = context->GetIsolate();
HandleScope handle_scope(isolate);

context->AllowCodeGenerationFromStrings(false);
context->SetEmbedderData(
ContextEmbedderIndex::kAllowCodeGenerationFromStrings, True(isolate));
context->SetEmbedderData(ContextEmbedderIndex::kAllowWasmCodeGeneration,
True(isolate));

Expand Down
1 change: 1 addition & 0 deletions src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,7 @@ class NoArrayBufferZeroFillScope {
V(inspector_console_extension_installer, v8::Function) \
V(inspector_disable_async_hooks, v8::Function) \
V(inspector_enable_async_hooks, v8::Function) \
V(maybe_cache_generated_source_map, v8::Function) \
V(messaging_deserialize_create_object, v8::Function) \
V(message_port, v8::Object) \
V(native_module_require, v8::Function) \
Expand Down
8 changes: 7 additions & 1 deletion src/node_context_data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,18 @@ namespace node {
#define NODE_BINDING_LIST_INDEX 36
#endif

#ifndef NODE_CONTEXT_ALLOW_CODE_GENERATION_FROM_STRINGS_INDEX
#define NODE_CONTEXT_ALLOW_CODE_GENERATION_FROM_STRINGS_INDEX 37
#endif

enum ContextEmbedderIndex {
kEnvironment = NODE_CONTEXT_EMBEDDER_DATA_INDEX,
kSandboxObject = NODE_CONTEXT_SANDBOX_OBJECT_INDEX,
kAllowWasmCodeGeneration = NODE_CONTEXT_ALLOW_WASM_CODE_GENERATION_INDEX,
kContextTag = NODE_CONTEXT_TAG,
kBindingListIndex = NODE_BINDING_LIST_INDEX
kBindingListIndex = NODE_BINDING_LIST_INDEX,
kAllowCodeGenerationFromStrings =
NODE_CONTEXT_ALLOW_CODE_GENERATION_FROM_STRINGS_INDEX
};

} // namespace node
Expand Down
6 changes: 5 additions & 1 deletion src/node_contextify.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,11 @@ MaybeLocal<Context> ContextifyContext::CreateV8Context(
ctx->Global());

Utf8Value name_val(env->isolate(), options.name);
ctx->AllowCodeGenerationFromStrings(options.allow_code_gen_strings->IsTrue());
// Delegate the code generation validation to
// node::ModifyCodeGenerationFromStrings.
ctx->AllowCodeGenerationFromStrings(false);
ctx->SetEmbedderData(ContextEmbedderIndex::kAllowCodeGenerationFromStrings,
options.allow_code_gen_strings);
ctx->SetEmbedderData(ContextEmbedderIndex::kAllowWasmCodeGeneration,
options.allow_code_gen_wasm);

Expand Down
43 changes: 43 additions & 0 deletions src/node_errors.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,38 @@ void OnFatalError(const char* location, const char* message) {
ABORT();
}

v8::ModifyCodeGenerationFromStringsResult ModifyCodeGenerationFromStrings(
bcoe marked this conversation as resolved.
Show resolved Hide resolved
v8::Local<v8::Context> context,
v8::Local<v8::Value> source,
bool is_code_like) {
HandleScope scope(context->GetIsolate());

Environment* env = Environment::GetCurrent(context);
if (env->source_maps_enabled()) {
// We do not expect the maybe_cache_generated_source_map to throw any more
// exceptions. If it does, just ignore it.
errors::TryCatchScope try_catch(env);
Local<Function> maybe_cache_source_map =
env->maybe_cache_generated_source_map();
Local<Value> argv[1] = {source};

MaybeLocal<Value> maybe_cached = maybe_cache_source_map->Call(
context, context->Global(), arraysize(argv), argv);
if (maybe_cached.IsEmpty()) {
DCHECK(try_catch.HasCaught());
}
}

Local<Value> allow_code_gen = context->GetEmbedderData(
ContextEmbedderIndex::kAllowCodeGenerationFromStrings);
bool codegen_allowed =
allow_code_gen->IsUndefined() || allow_code_gen->IsTrue();
return {
codegen_allowed,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If no callback is provided, is codegen_allowed just always truthy?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the value set in v8::Context::AllowCodeGenerationFromStrings is true, no host-callbacks are called on compiling dynamic sources.

The callback is called only when AllowCodeGenerationFromStrings(false) is set on the context. Thus if the callback is not provided when AllowCodeGenerationFromStrings is set to false, no dynamic sources are allowed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior to this, did v8::Context::AllowCodeGenerationFromStrings default to true?

Any concerns about performance hits we might observe now that we're calling a handler? Perhaps it would be worth adding a benchmark?

Copy link
Member Author

@legendecas legendecas Jul 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bcoe thank you for your suggestion. I added a new fast-path on if sourcemap is enabled in ModifyCodeGenerationFromStrings and ran a benchmark against the main branch. Now the difference when sourcemap is not enabled can be slight if the dynamic code is not trivial.

However, with sourcemap enabled, the difference can be significant. But I suppose this is acceptable since sourcemap comes with costs.

                                                confidence improvement accuracy (*)   (**)  (***)
es/eval.js n=1000000 method='without-sourcemap'        ***     -1.54 %       ±0.36% ±0.49% ±0.63%
es/eval.js n=1000000 method='sourcemap'        ***    -31.90 %       ±1.41% ±1.89% ±2.49%

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for adding the benchmark 👌

I'm curious, how much more does it slow down if you have the noop closure, without the fast path?

However, with sourcemap enabled, the difference can be significant. But I suppose this is acceptable since sourcemap comes with costs.

I think this is to be expected. Unrelated to this PR, I'm becoming convinced we should stop performing the logic that loads the original source from disk for providing visual context, as I believe this is one of the major slow downs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The slowdown can be up to ~25% without the fast path with this micro benchmark.

I think this is to be expected. Unrelated to this PR, I'm becoming convinced we should stop performing the logic that loads the original source from disk for providing visual context, as I believe this is one of the major slow downs.

Yeah, when sourcemap support is not enabled, the visual context is not prepended to the value of the stack property. It's just being prepended when the error is being printed by the runtime like fatal exceptions. While accessing stack property is one of the most commonly used paths, improving the performance of it is always a good motivation. Are you planning to work on it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you planning to work on it?

I don't think I'll have cycles to work on this in the short term, I'll CC you on the pertinent issue.

{},
};
}

namespace errors {

TryCatchScope::~TryCatchScope() {
Expand Down Expand Up @@ -836,6 +868,13 @@ static void SetSourceMapsEnabled(const FunctionCallbackInfo<Value>& args) {
env->set_source_maps_enabled(args[0].As<Boolean>()->Value());
}

static void SetMaybeCacheGeneratedSourceMap(
const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
CHECK(args[0]->IsFunction());
env->set_maybe_cache_generated_source_map(args[0].As<Function>());
}

static void SetEnhanceStackForFatalException(
const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Expand Down Expand Up @@ -870,6 +909,7 @@ static void TriggerUncaughtException(const FunctionCallbackInfo<Value>& args) {
void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
registry->Register(SetPrepareStackTraceCallback);
registry->Register(SetSourceMapsEnabled);
registry->Register(SetMaybeCacheGeneratedSourceMap);
registry->Register(SetEnhanceStackForFatalException);
registry->Register(NoSideEffectsToString);
registry->Register(TriggerUncaughtException);
Expand All @@ -883,6 +923,9 @@ void Initialize(Local<Object> target,
env->SetMethod(
target, "setPrepareStackTraceCallback", SetPrepareStackTraceCallback);
env->SetMethod(target, "setSourceMapsEnabled", SetSourceMapsEnabled);
env->SetMethod(target,
"setMaybeCacheGeneratedSourceMap",
SetMaybeCacheGeneratedSourceMap);
env->SetMethod(target,
"setEnhanceStackForFatalException",
SetEnhanceStackForFatalException);
Expand Down
5 changes: 5 additions & 0 deletions src/node_errors.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,11 @@ void DecorateErrorStack(Environment* env,
const errors::TryCatchScope& try_catch);
} // namespace errors

v8::ModifyCodeGenerationFromStringsResult ModifyCodeGenerationFromStrings(
v8::Local<v8::Context> context,
v8::Local<v8::Value> source,
bool is_code_like);

} // namespace node

#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
Expand Down
8 changes: 8 additions & 0 deletions test/message/source_map_eval.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Flags: --enable-source-maps

'use strict';
require('../common');
const fs = require('fs');

const content = fs.readFileSync(require.resolve('../fixtures/source-map/tabs.js'), 'utf8');
eval(content);
12 changes: 12 additions & 0 deletions test/message/source_map_eval.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
ReferenceError: alert is not defined
at Object.eval (*tabs.coffee:26:2)
at eval (*tabs.coffee:1:14)
at Object.<anonymous> (*source_map_eval.js:8:1)
at Module._compile (node:internal/modules/cjs/loader:*)
at Module._extensions..js (node:internal/modules/cjs/loader:*)
at Module.load (node:internal/modules/cjs/loader:*)
at Module._load (node:internal/modules/cjs/loader:*)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:*)
at node:internal/main/run_main_module:*

Node.js *