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

vm: add bindings for v8::CompileFunctionInContext #21571

Closed
wants to merge 1 commit into from
Closed

vm: add bindings for v8::CompileFunctionInContext #21571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 28 additions & 0 deletions doc/api/vm.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -637,6 +637,34 @@ console.log(globalVar);
// 1000
```

## vm.compileFunction(code[, params[, options]])
<!-- YAML
added: REPLACEME
-->
* `code` {string} The body of the function to compile.
* `params` {string[]} An array of strings containing all parameters for the
function.
* `options` {Object}
* `filename` {string} Specifies the filename used in stack traces produced
by this script. **Default:** `''`.
* `lineOffset` {number} Specifies the line number offset that is displayed
in stack traces produced by this script. **Default:** `0`.
* `columnOffset` {number} Specifies the column number offset that is displayed
in stack traces produced by this script. **Default:** `0`.
* `cachedData` {Buffer} Provides an optional `Buffer` with V8's code cache
data for the supplied source.
* `produceCachedData` {boolean} Specifies whether to produce new cache data.
**Default:** `false`.
* `parsingContext` {Object} The sandbox/context in which the said function
should be compiled in.
* `contextExtensions` {Object[]} An array containing a collection of context
extensions (objects wrapping the current scope) to be applied while
compiling. **Default:** `[]`.

Compiles the given code into the provided context/sandbox (if no context is
supplied, the current context is used), and returns it wrapped inside a
function with the given `params`.

## vm.createContext([sandbox[, options]])
<!-- YAML
added: v0.3.1
Expand Down
94 changes: 94 additions & 0 deletions lib/vm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,17 @@ const {
ContextifyScript,
makeContext,
isContext: _isContext,
compileFunction: _compileFunction
} = internalBinding('contextify');

const { ERR_INVALID_ARG_TYPE } = require('internal/errors').codes;
const { isUint8Array } = require('internal/util/types');
const { validateInt32, validateUint32 } = require('internal/validators');
const kParsingContext = Symbol('script parsing context');

const ArrayForEach = Function.call.bind(Array.prototype.forEach);
const ArrayIsArray = Array.isArray;

class Script extends ContextifyScript {
constructor(code, options = {}) {
code = `${code}`;
Expand Down Expand Up @@ -286,6 +291,94 @@ function runInThisContext(code, options) {
return createScript(code, options).runInThisContext(options);
}

function compileFunction(code, params, options = {}) {
if (typeof code !== 'string') {
throw new ERR_INVALID_ARG_TYPE('code', 'string', code);
}
if (params !== undefined) {
if (!ArrayIsArray(params)) {
throw new ERR_INVALID_ARG_TYPE('params', 'Array', params);
}
ArrayForEach(params, (param, i) => {
if (typeof param !== 'string') {
throw new ERR_INVALID_ARG_TYPE(`params[${i}]`, 'string', param);
}
});
}

Copy link
Member

@jdalton jdalton Jul 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

👆 I know many internals try to avoid prototypal methods directly (opting to create generic forms). Should this be done for forEach use?

Update:

Same with isArray (e.g. const { isArray } = Array)

Copy link
Member

Choose a reason for hiding this comment

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

We've generally tried to avoid using forEach within core functions because it's an order of magnitude slower than a regular for-loop. Even if this is not expected to be a hot function, I think I'd rather see this as a regular for-loop.

Copy link
Member

Choose a reason for hiding this comment

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

if we're taking the performance approach, forEach has actually become faster in the common case than a for loop (in V8 anyway).

Copy link
Member

Choose a reason for hiding this comment

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

👆 \cc @bmeurer

Copy link
Member

Choose a reason for hiding this comment

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

Whatever the outcome of this discussion, please let’s not hold up the PR over this – this isn’t hot code (or at least the cost of actual parsing + compilation is likely going to overshadow any choice of this kind), so I think it’s fine to go with whatever the PR author considers most readable.

const {
filename = '',
columnOffset = 0,
lineOffset = 0,
cachedData = undefined,
produceCachedData = false,
parsingContext = undefined,
contextExtensions = [],
} = options;

if (typeof filename !== 'string') {
throw new ERR_INVALID_ARG_TYPE('options.filename', 'string', filename);
}
validateUint32(columnOffset, 'options.columnOffset');
validateUint32(lineOffset, 'options.lineOffset');
if (cachedData !== undefined && !isUint8Array(cachedData)) {
throw new ERR_INVALID_ARG_TYPE(
'options.cachedData',
'Uint8Array',
cachedData
);
}
if (typeof produceCachedData !== 'boolean') {
throw new ERR_INVALID_ARG_TYPE(
'options.produceCachedData',
'boolean',
produceCachedData
);
}
if (parsingContext !== undefined) {
if (
typeof parsingContext !== 'object' ||
parsingContext === null ||
!isContext(parsingContext)
) {
throw new ERR_INVALID_ARG_TYPE(
'options.parsingContext',
'Context',
parsingContext
);
}
}
if (!ArrayIsArray(contextExtensions)) {
throw new ERR_INVALID_ARG_TYPE(
'options.contextExtensions',
'Array',
contextExtensions
);
}
ArrayForEach(contextExtensions, (extension, i) => {
if (typeof extension !== 'object') {
throw new ERR_INVALID_ARG_TYPE(
`options.contextExtensions[${i}]`,
'object',
extension
);
}
});

return _compileFunction(
code,
filename,
lineOffset,
columnOffset,
cachedData,
produceCachedData,
parsingContext,
contextExtensions,
params
);
}


module.exports = {
Script,
createContext,
Expand All @@ -294,6 +387,7 @@ module.exports = {
runInNewContext,
runInThisContext,
isContext,
compileFunction,
};

if (process.binding('config').experimentalVMModules) {
Expand Down
139 changes: 139 additions & 0 deletions src/node_contextify.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ void ContextifyContext::Init(Environment* env, Local<Object> target) {

env->SetMethod(target, "makeContext", MakeContext);
env->SetMethod(target, "isContext", IsContext);
env->SetMethod(target, "compileFunction", CompileFunction);
}


Expand Down Expand Up @@ -985,6 +986,144 @@ class ContextifyScript : public BaseObject {
};


void ContextifyContext::CompileFunction(
const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Isolate* isolate = env->isolate();
Local<Context> context = env->context();

// Argument 1: source code
CHECK(args[0]->IsString());
Local<String> code = args[0].As<String>();

// Argument 2: filename
CHECK(args[1]->IsString());
Local<String> filename = args[1].As<String>();

// Argument 3: line offset
CHECK(args[2]->IsNumber());
Local<Integer> line_offset = args[2].As<Integer>();
Copy link
Member

Choose a reason for hiding this comment

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

have it been tested these offsets actually do anything for compiling functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea, my guess is that we set them to 0 for like... 99% cases, but I put them in just for parity with ContextifyScript::New.

Copy link
Member

@devsnek devsnek Jun 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

well in ContextifyScript the entire source can be offset for dealing with source positions in html bodies and such. if they don't do anything here it would be best to remove them. (easy test is offset by a bunch and make the body have a syntax error, then read the column offset in the stack)

Copy link
Member

Choose a reason for hiding this comment

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

did you ever verify this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't, sorry.

@addaleax @hashseed is it worth putting these in just for the sake of parity?

Copy link
Member

Choose a reason for hiding this comment

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

I think this could be useful.

One way to test this is to throw an exception ans check whether the exception position is affected by the offsets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It indeed works. Will add tests.

Copy link
Member

Choose a reason for hiding this comment

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

The Number type is indeed checked in JavaScript, but not that the number is an integer. That should be added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, will throw in Number.isInteger checks.

Copy link
Member

Choose a reason for hiding this comment

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

BTW you should use validateInteger() in internal/validators.js.


// Argument 4: column offset
CHECK(args[3]->IsNumber());
Local<Integer> column_offset = args[3].As<Integer>();

This comment was marked as resolved.

Sign in to view


// Argument 5: cached data (optional)
Local<Uint8Array> cached_data_buf;
if (!args[4]->IsUndefined()) {
CHECK(args[4]->IsUint8Array());
Copy link
Member

Choose a reason for hiding this comment

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

This should accept all ArrayBuffer view types, and ArrayBuffer itself, consistent with other newer APIs. Hint: use Buffer::HasInstance()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am kinda split if I should use Buffers or stick with what we have, also because it's consistent with Script::New. @hashseed @addaleax would love to hear your thoughts on this.

Copy link
Member

Choose a reason for hiding this comment

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

I’d stick with consistency with Script::New, and maybe relax this in a later PR for both of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds great to me. @TimothyGu wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

Sure.

cached_data_buf = args[4].As<Uint8Array>();
}

// Argument 6: produce cache data
CHECK(args[5]->IsBoolean());
bool produce_cached_data = args[5]->IsTrue();

// Argument 7: parsing context (optional)
Local<Context> parsing_context;
if (!args[6]->IsUndefined()) {
CHECK(args[6]->IsObject());
ContextifyContext* sandbox =
ContextifyContext::ContextFromContextifiedSandbox(
env, args[6].As<Object>());
CHECK_NOT_NULL(sandbox);
parsing_context = sandbox->context();
} else {
parsing_context = context;
}

// Argument 8: context extensions (optional)
Local<Array> context_extensions_buf;
if (!args[7]->IsUndefined()) {
CHECK(args[7]->IsArray());
context_extensions_buf = args[7].As<Array>();
}

// Argument 9: params for the function (optional)
Local<Array> params_buf;
if (!args[8]->IsUndefined()) {
CHECK(args[8]->IsArray());
params_buf = args[8].As<Array>();
}

// Read cache from cached data buffer
ScriptCompiler::CachedData* cached_data = nullptr;
if (!cached_data_buf.IsEmpty()) {
ArrayBuffer::Contents contents = cached_data_buf->Buffer()->GetContents();
uint8_t* data = static_cast<uint8_t*>(contents.Data());
cached_data = new ScriptCompiler::CachedData(
data + cached_data_buf->ByteOffset(), cached_data_buf->ByteLength());
Copy link
Member

Choose a reason for hiding this comment

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

Buffer methods should simplify this quite a bit, as they take care of byte offsets.

Copy link
Member

Choose a reason for hiding this comment

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

This hasn't been fixed yet. Note, you don't need to support all types of ArrayBufferView to use Buffer methods. In fact you can do a direct switch of data + cached_data_buf->ByteOffset() by Buffer::Data(cached_data_buf) and cached_data_buf->ByteLength() by Buffer::Length(cached_data_buf).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized that, and had been planning to change these in both CompileFunction and Script::New in a quick subsequent PR. I could make one right now, if you'd like.

Or would you still prefer that I make the switch in this one and just change Script::New in the other PR?

Copy link
Member

Choose a reason for hiding this comment

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

Nah, do as you planned :)

}

ScriptOrigin origin(filename, line_offset, column_offset);
ScriptCompiler::Source source(code, origin, cached_data);
ScriptCompiler::CompileOptions options;
if (source.GetCachedData() == nullptr) {
options = ScriptCompiler::kNoCompileOptions;
} else {
options = ScriptCompiler::kConsumeCodeCache;
}

TryCatch try_catch(isolate);
Context::Scope scope(parsing_context);

// Read context extensions from buffer
std::vector<Local<Object>> context_extensions;
if (!context_extensions_buf.IsEmpty()) {
for (uint32_t n = 0; n < context_extensions_buf->Length(); n++) {
Local<Value> val;
if (!context_extensions_buf->Get(context, n).ToLocal(&val)) return;
CHECK(val->IsObject());
context_extensions.push_back(val.As<Object>());
}
}

// Read params from params buffer
std::vector<Local<String>> params;
if (!params_buf.IsEmpty()) {
for (uint32_t n = 0; n < params_buf->Length(); n++) {
Local<Value> val;
if (!params_buf->Get(context, n).ToLocal(&val)) return;
CHECK(val->IsString());
params.push_back(val.As<String>());
}
}

MaybeLocal<Function> maybe_fun = ScriptCompiler::CompileFunctionInContext(
context, &source, params.size(), params.data(),
context_extensions.size(), context_extensions.data(), options);

Local<Function> fun;
if (maybe_fun.IsEmpty() || !maybe_fun.ToLocal(&fun)) {
ContextifyScript::DecorateErrorStack(env, try_catch);
try_catch.ReThrow();
return;
}

if (produce_cached_data) {
const std::unique_ptr<ScriptCompiler::CachedData>
cached_data(ScriptCompiler::CreateCodeCacheForFunction(fun, code));
bool cached_data_produced = cached_data != nullptr;
if (cached_data_produced) {
MaybeLocal<Object> buf = Buffer::Copy(
env,
reinterpret_cast<const char*>(cached_data->data),
cached_data->length);
if (fun->Set(
parsing_context,
env->cached_data_string(),
buf.ToLocalChecked()).IsNothing()) return;
}
if (fun->Set(
Copy link
Member

Choose a reason for hiding this comment

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

what if the return value was something like { function, codeCache }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That could be rad, but I'm supposing that most people would just be expecting the function or would disable the cache and then we'll be returning { function }, which doesn't look super good?

Copy link
Member

Choose a reason for hiding this comment

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

Like mentioned previously, let's not have this at all, and expose ScriptCompiler::CreateCodeCacheForFunction in a separate binding like we already do for ScriptCompiler::CreateCodeCache.

parsing_context,
env->cached_data_produced_string(),
Boolean::New(isolate, cached_data_produced)).IsNothing()) return;
}

args.GetReturnValue().Set(fun);
}


void Initialize(Local<Object> target,
Local<Value> unused,
Local<Context> context) {
Expand Down
2 changes: 2 additions & 0 deletions src/node_contextify.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ class ContextifyContext {
private:
static void MakeContext(const v8::FunctionCallbackInfo<v8::Value>& args);
static void IsContext(const v8::FunctionCallbackInfo<v8::Value>& args);
static void CompileFunction(
const v8::FunctionCallbackInfo<v8::Value>& args);
static void WeakCallback(
const v8::WeakCallbackInfo<ContextifyContext>& data);
static void PropertyGetterCallback(
Expand Down
Loading