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.

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 Script.createCodeCache() #20300

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 9 additions & 0 deletions doc/api/deprecations.md
Original file line number Diff line number Diff line change
Expand Up @@ -1005,6 +1005,14 @@ accepted by the legacy `url.parse()` API. The mentioned APIs now use the WHATWG
URL parser that requires strictly valid URLs. Passing an invalid URL is
deprecated and support will be removed in the future.

<a id="DEP00XX"></a>
### DEP00XX: vm.Script cached data

Type: Documentation-only
Copy link
Member

Choose a reason for hiding this comment

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

Any reason this isn't runtime?

Copy link
Member

Choose a reason for hiding this comment

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

@TimothyGu Some previous comments on that in #20300 (comment) … for a caching functionality, there is no reason to introduce a runtime deprecation, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

i never know when to make something documentation or runtime... do we have some sort of guide on deciding which to do? i always kinda saw documentation as things that are too ingrained in npm packages such that like any project would be spammed if it used a runtime dep

Copy link
Member

Choose a reason for hiding this comment

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

@devsnek I don’t know if you’ve read it, but https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#deprecations might help you?

Generally, we should avoid moving directly from a documented feature towards a runtime deprecation. But yes, obviously popularity of a feature has played a role in deciding how fast we move with those steps…

Copy link
Member Author

Choose a reason for hiding this comment

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

@addaleax i've seen that but it doesn't really explain when to use them, just how they fit into semver. is the generally expected process that something will do docs -> runtime -> eol?

Copy link
Member

Choose a reason for hiding this comment

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

is the generally expected process that something will do docs -> runtime -> eol?

Yes, unless we have reason to deviate from it. But in this case I do think we have reason to do so. :)


The option `produceCachedData` has been deprecated. Use
[`script.createCachedData()`][] instead.

[`--pending-deprecation`]: cli.html#cli_pending_deprecation
[`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size
[`Buffer.from(array)`]: buffer.html#buffer_class_method_buffer_from_array
Expand Down Expand Up @@ -1055,6 +1063,7 @@ deprecated and support will be removed in the future.
[`process.env`]: process.html#process_process_env
[`punycode`]: punycode.html
[`require.extensions`]: modules.html#modules_require_extensions
[`script.createCachedData()`]: vm.html#vm_script_create_cached_data
[`setInterval()`]: timers.html#timers_setinterval_callback_delay_args
[`setTimeout()`]: timers.html#timers_settimeout_callback_delay_args
[`tls.CryptoStream`]: tls.html#tls_class_cryptostream
Expand Down
32 changes: 32 additions & 0 deletions doc/api/vm.md
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,10 @@ changes:
pr-url: https://github.com/nodejs/node/pull/4777
description: The `cachedData` and `produceCachedData` options are
supported now.
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/20300
description: The `produceCachedData` is deprecated in favour of
Copy link
Contributor

Choose a reason for hiding this comment

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

favour -> favor

`script.createCachedData()`

This comment was marked as resolved.

-->

* `code` {string} The JavaScript code to compile.
Expand All @@ -431,11 +435,39 @@ changes:
`cachedData` property of the returned `vm.Script` instance.
The `cachedDataProduced` value will be set to either `true` or `false`
depending on whether code cache data is produced successfully.
This option is deprecated in favor of `script.createCachedData`.

Creating a new `vm.Script` object compiles `code` but does not run it. The
compiled `vm.Script` can be run later multiple times. The `code` is not bound to
any global object; rather, it is bound before each run, just for that run.

### script.createCachedData()
<!-- YAML
added: REPLACEME
-->

* Returns: {Buffer}

Creates a code cache that can be used with the Script constructor's
Copy link
Contributor

Choose a reason for hiding this comment

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

Script in backticks?

`cachedData` option. Returns a Buffer. This method may be called at any
time and any number of times.

```js
const script = new vm.Script(`
function add(a, b) {
return a + b;
}
const x = add(1, 2);
`);

const cacheWithoutX = script.createCachedData();

script.runInThisContext();

const cacheWithX = script.createCachedData();
```

### script.runInContext(contextifiedSandbox[, options])
<!-- YAML
added: v0.3.1
Expand Down
26 changes: 24 additions & 2 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "base_object-inl.h"
#include "node_contextify.h"
#include "node_context_data.h"
#include "node_errors.h"

namespace node {
namespace contextify {
Expand Down Expand Up @@ -596,6 +597,7 @@ class ContextifyScript : public BaseObject {
Local<FunctionTemplate> script_tmpl = env->NewFunctionTemplate(New);
script_tmpl->InstanceTemplate()->SetInternalFieldCount(1);
script_tmpl->SetClassName(class_name);
env->SetProtoMethod(script_tmpl, "createCachedData", CreateCachedData);
env->SetProtoMethod(script_tmpl, "runInContext", RunInContext);
env->SetProtoMethod(script_tmpl, "runInThisContext", RunInThisContext);

Expand Down Expand Up @@ -637,7 +639,7 @@ class ContextifyScript : public BaseObject {
Local<Context> parsing_context = context;

if (argc > 2) {
// new ContextifyScript(code, filename, lineOffset, columnOffset
// new ContextifyScript(code, filename, lineOffset, columnOffset,
// cachedData, produceCachedData, parsingContext)
CHECK_EQ(argc, 7);
CHECK(args[2]->IsNumber());
Expand Down Expand Up @@ -719,7 +721,7 @@ class ContextifyScript : public BaseObject {
Boolean::New(isolate, source.GetCachedData()->rejected));
} else if (produce_cached_data) {
const ScriptCompiler::CachedData* cached_data =
ScriptCompiler::CreateCodeCache(v8_script.ToLocalChecked(), code);
ScriptCompiler::CreateCodeCache(v8_script.ToLocalChecked());
bool cached_data_produced = cached_data != nullptr;
if (cached_data_produced) {
MaybeLocal<Object> buf = Buffer::Copy(
Expand All @@ -745,6 +747,26 @@ class ContextifyScript : public BaseObject {
}


static void CreateCachedData(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
ContextifyScript* wrapped_script;
ASSIGN_OR_RETURN_UNWRAP(&wrapped_script, args.Holder());
Local<UnboundScript> unbound_script =
PersistentToLocal(env->isolate(), wrapped_script->script_);
std::unique_ptr<ScriptCompiler::CachedData> cached_data(
ScriptCompiler::CreateCodeCache(unbound_script));
if (!cached_data) {
args.GetReturnValue().Set(Buffer::New(env, 0).ToLocalChecked());
} else {
MaybeLocal<Object> buf = Buffer::Copy(
env,
reinterpret_cast<const char*>(cached_data->data),
cached_data->length);
args.GetReturnValue().Set(buf.ToLocalChecked());

This comment was marked as resolved.

Copy link
Member

@joyeecheung joyeecheung Jun 13, 2018

Choose a reason for hiding this comment

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

It may be a good idea to throw ERR_BUFFER_TOO_LARGE if cached_data->length > Buffer::kMaxLength before Buffer::Copy and ERR_MEMORY_ALLOCATION_FAILED if buf.IsEmpty() is true instead of doing buf.ToLocalChecked()

}
}


static void RunInThisContext(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

Expand Down
22 changes: 22 additions & 0 deletions test/parallel/test-vm-createcacheddata.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
'use strict';

require('../common');

const { Script } = require('vm');
const assert = require('assert');

const source = 'function x() {} const y = x();';

const script = new Script(source);
let cachedData = script.createCachedData();
assert(cachedData instanceof Buffer);

assert(!new Script(source, { cachedData }).cachedDataRejected);

script.runInNewContext();

for (let i = 0; i < 10; i += 1) {
cachedData = script.createCachedData();

assert(!new Script(source, { cachedData }).cachedDataRejected);
}