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
wants to merge 1 commit into
base: master
from

Conversation

@devsnek
Member

devsnek commented Apr 25, 2018

Closes #20052

/cc @nodejs/vm

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@devsnek devsnek requested a review from hashseed Apr 25, 2018

@devsnek devsnek added vm and removed lib / src labels Apr 25, 2018

lib/vm.js Outdated
if (cachedData && !cachedDataWarned) {
process.emitWarning(cachedDataMessage, 'DeprecationWarning', 'DEPXXXX');
cachedDataWarned = true;
}

This comment has been minimized.

@addaleax

addaleax Apr 25, 2018

Member

Note that this makes this PR semver-major – it might be easier to split the deprecation out?

Also – What’s the motivation for not doing a docs-only deprecation first? Do we actually want a run-time deprecation at any point? It seems like, for cache information, just making them no-ops would be less breakage for users…

This comment has been minimized.

@devsnek

devsnek Apr 25, 2018

Member

i can remove the runtime dep

This comment has been minimized.

@jasnell

jasnell Apr 27, 2018

Member

The additional of a new throw also makes it semver-major.

Wait, nevermind, this is a check on a new property. Scratch that :-)

Type: Documentation-only
The options `produceCachedData` and `cachedData` have been deprecated. Please use

This comment has been minimized.

@hashseed

hashseed Apr 26, 2018

Member

I would also support splitting the deprecation into a separate PR, so that we can ship this earlier without semver changes.

This comment has been minimized.

@devsnek

devsnek Apr 26, 2018

Member

I changed it to docs only, not sure why I made it runtime in the first place

This comment has been minimized.

@jasnell

jasnell Apr 27, 2018

Member

Even as a docs-only deprecation, it should be separated out into a separate commit.

This comment has been minimized.

@jasnell

jasnell Apr 27, 2018

Member

Also ... just a nit... s/DEPXXXX/DEP00XX ... the release tooling looks specifically for the DEP00XX pattern to ensure that the codes are properly assigned before release.

env,
reinterpret_cast<const char*>(cached_data->data),
cached_data->length);
args.GetReturnValue().Set(buf.ToLocalChecked());

This comment was marked as resolved.

@hashseed

hashseed Apr 26, 2018

Member

I think you need to delete cache_data. Or even better, use unique_ptr around it.

src/env.h Outdated
V(change_string, "change") \
V(channel_string, "channel") \
V(chunks_sent_since_last_write_string, "chunksSentSinceLastWrite") \
V(code_cache_used_string, "codeCacheUsed") \

This comment has been minimized.

@hashseed

hashseed Apr 26, 2018

Member

This is an API change. Not sure whether that belongs into a separate PR? Not sure if this would block this PR from going into Node 10 otherwise.

* `codeCache` {Buffer} Provides an optional `Buffer` with V8's code cache
data for the supplied source. When supplied, the `codeCacheRejected` value
will be set to either `true` or `false` depending on acceptance of the data
by V8.
* `cachedData` {Buffer} Provides an optional `Buffer` with V8's code cache

This comment has been minimized.

@TimothyGu

TimothyGu Apr 26, 2018

Member

Is the code cache generated by createCodeCache compatible with cachedData

This comment has been minimized.

@hashseed

hashseed Apr 26, 2018

Member

Yes. The latter is produced with createCodeCache.

This comment has been minimized.

@TimothyGu

TimothyGu Apr 26, 2018

Member

In that case I’m reluctant to deprecate an older name of the same thing. Actually would rather change createCodeCache to createCachedData or something similar.

@joyeecheung

This comment has been minimized.

Member

joyeecheung commented Apr 27, 2018

@hashseed Can the code cache generated by a different version of v8 be loaded by another? (My guess is no?)

@hashseed

This comment has been minimized.

Member

hashseed commented Apr 27, 2018

We check a version hash before deserializing. If the version hash mismatches, V8 rejects the cache data.

@joyeecheung

This comment has been minimized.

Member

joyeecheung commented Apr 27, 2018

@hashseed Thanks. It would be good to document about this (otherwise the user only has a boolean codeCacheRejected without further information or possible cause)

@devsnek

This comment has been minimized.

Member

devsnek commented Apr 27, 2018

I'm going to work on some patches this weekend for v8 to provide more data on what happened with the caches besides just if it was rejected, which should help make this API a bit better

@hashseed

This comment has been minimized.

Member

hashseed commented Apr 28, 2018

Generally agree. Though I think landing the minimal change of adding this new API without adding the reject reason or deprecating the old API would make sense too.

@TimothyGu

Code looks good.

-->
Creates a code cache that can be used with the Script constructor's
`cachedData` option. Returns a Buffer. If the code cache cannot be created then

This comment was marked as resolved.

@TimothyGu

TimothyGu Apr 28, 2018

Member
* Returns: {Buffer}
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/20300
description: The `produceCachedData` is deprecated in favour of
`script.createCachedData()`

This comment was marked as resolved.

@TimothyGu

TimothyGu Apr 28, 2018

Member

You need to say this in the actual option documentation as well.

Creates a code cache that can be used with the Script constructor's
`cachedData` option. Returns a Buffer. If the code cache cannot be created then
`ERR_CODE_CACHE_CREATION_FAILED` is thrown. This method may be called at any

This comment has been minimized.

@TimothyGu

TimothyGu Apr 28, 2018

Member

@hashseed What can possibly lead to failure of creating cached data? It would be helpful here for the documentation.

Creates a code cache that can be used with the Script constructor's
`cachedData` option. Returns a Buffer. If the code cache cannot be created then
`ERR_CODE_CACHE_CREATION_FAILED` is thrown. This method may be called at any
time and any number of times.

This comment has been minimized.

@TimothyGu

TimothyGu Apr 28, 2018

Member

Maybe also link to the V8 blog post?

if (script.cachedDataProduced)
data = script.cachedData.toString('base64');

This comment has been minimized.

@TimothyGu

TimothyGu Apr 28, 2018

Member

The old code should still be tested.

<a id="DEP00XX"></a>
### DEP00XX: vm.Script cached data
Type: Documentation-only

This comment has been minimized.

@TimothyGu

TimothyGu Apr 28, 2018

Member

Any reason this isn't runtime?

This comment has been minimized.

@addaleax

addaleax Apr 28, 2018

Member

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

This comment has been minimized.

@devsnek

devsnek Apr 28, 2018

Member

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

This comment has been minimized.

@addaleax

addaleax Apr 28, 2018

Member

@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…

This comment has been minimized.

@devsnek

devsnek Apr 28, 2018

Member

@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?

This comment has been minimized.

@addaleax

addaleax Apr 29, 2018

Member

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. :)

@hashseed

This comment has been minimized.

Member

hashseed commented Apr 29, 2018

These here are possible reject reasons.

@TimothyGu

This comment has been minimized.

Member

TimothyGu commented Apr 29, 2018

@hashseed I meant possible reasons for failure of creating a code cache, not consuming one :)

@hashseed

This comment has been minimized.

Member

hashseed commented Apr 30, 2018

Oh. Creating may fail if the script is actually an asm.js module or if the debugger is active. The latter can probably be fixed as soon as the debug context has been removed completely.

@jdalton

This comment has been minimized.

Member

jdalton commented May 28, 2018

Any updates here? What's the status?

@devsnek

This comment has been minimized.

Member

devsnek commented May 28, 2018

@jdalton hashseed is going to backport a bunch of changes to SharedFunctionInfo soonish, this is just waiting for that.

@hashseed

This comment has been minimized.

Member

hashseed commented May 29, 2018

Umm.. wait. Which changes am I supposed to backport? I am not aware of anything actionable on my end. Did I drop the ball?

@devsnek

This comment has been minimized.

Member

devsnek commented May 29, 2018

@hashseed a couple weeks ago I emailed you about backporting the changes for this and modules sfi and my understanding from that was you would be backporting the changes. if that's wrong sorry about the misunderstanding 😅

@hashseed

This comment has been minimized.

Member

hashseed commented May 29, 2018

I haven't found such an email unfortunately. Do you mean by any chance this change? I'm pessimistic that it will get green light for back porting since this is not a bug fix. We could just float this in Node?

@jdalton

This comment has been minimized.

Member

jdalton commented Jun 1, 2018

@devsnek Are you cool floating the change into Node?

@nodejs nodejs deleted a comment from refack Jun 1, 2018

@devsnek

This comment has been minimized.

Member

devsnek commented Jun 1, 2018

CI https://ci.nodejs.org/job/node-test-pull-request/15219/

edit: i thought hashseed's pr landed whoopsie

@jdalton

This comment has been minimized.

Member

jdalton commented Jun 11, 2018

@devsnek

i thought hashseed's pr landed whoopsie

Which PR?

Update:

Ah, this one (and it landed) :D

@jdalton

This comment has been minimized.

Member

jdalton commented Jun 13, 2018

Yes, that’s why I’d lean towards an empty buffer, especially because it’s already something that doesn’t happen most of the time

Okay, I can dig that too.

@jdalton

This comment has been minimized.

Member

jdalton commented Jun 19, 2018

Stepping into this I think a little review would be good. Here is how things are done today.

const script = new vm.Script(content, {
  cachedData, // may be populated with a buffer or may be `undefined`
  filename,
  produceCachedData: true
})

let scriptData
let changed = false

if (! cachedData &&
    script.cachedData) { 
  changed = true
  scriptData = script.cachedData
} else if (cachedData &&
    script.cachedDataRejected) {
  changed = true
}

if (changed) {
  if (scriptData) {
    writeFile(cacheFilename, scriptData)
  } else {
    removeFile(cacheFilename)
  }
}

Here there is no error thrown if the cacheData cannot be produced. It simply doesn't populate the script.cachedData value. Transitioning to script.createCachedData() (maybe call it getCachedData()) I would expect a similar experience (which is why I proposed returning a buffer or null).

const script = new vm.Script(content, {
  cachedData, // may be populated with a buffer or may be `undefined`
  filename
})

const scriptData = script.getCachedData()

let changed = false

if (! cachedData &&
    scriptData) { 
  changed = true
} else if (cachedData &&
    script.cachedDataRejected) {
  changed = true
}

if (changed) {
  if (scriptData) {
    writeFile(cacheFilename, scriptData)
  } else {
    removeFile(cacheFilename)
  }
}

An interesting note is that today if cachedData is provided to vm.Script() then it won't produce a script.cachedData result. I could see that being carried over to script.getCachedData() too _(it would return null).

@devsnek

This comment has been minimized.

Member

devsnek commented Jun 19, 2018

@jdalton right now it returns an empty buffer. i disagree with disabling it because this is a different usage. the api that can produce cached data at any time has an inherent time state we can't match up.

the usage i originally imagined was more like

let cachedData;
try {
  catchedData = await readFile('./x');
} catch {}

let script = new vm.Script(..., {
  cachedData,
  ...
});

// ... run script or don't run script whatever get it to the state you want

if (!cachedData) {
  // with error
  try {
    await writeFile('./x', script.createCachedData());
  } catch {}

  // without error
  const buf = script.createCachedData();
  if (buf.length) {
//if (buf) {
    await writeFile('./x', buf);
  }
}
@jdalton

This comment has been minimized.

Member

jdalton commented Jun 19, 2018

Okay! :shipit:

@jdalton

This comment has been minimized.

assert(cachedData instanceof Buffer);
data = cachedData.toString('base64');

This comment has been minimized.

@TimothyGu

TimothyGu Jun 21, 2018

Member

This overwrites data created earlier. Is it possible to test the buffer returned from both produceCachedData and createCachedData?

@jdalton

This comment was marked as outdated.

Member

jdalton commented Jun 21, 2018

The failing tests look unrelated.

On the travis they point to:

Error: ENOSPC: no space left on device, watch '/home/travis/build/nodejs/node/test/.tmp.1/watch1/foo'

On jenkins they point to:

java.lang.OutOfMemoryError: Java heap space

and

tap2junit: error: argument --input/-i: can't open 'test.tap': [Errno 2] No such file or directory: 'test.tap'
POST BUILD TASK : FAILURE
@jdalton

This comment has been minimized.

@addaleax addaleax added the C++ label Jun 22, 2018

@addaleax

This comment has been minimized.

Member

addaleax commented Jun 22, 2018

@jdalton

This comment has been minimized.

Member

jdalton commented Jun 22, 2018

While experimenting with createCachedData() locally I noticed the produced buffers are roughly the same size as the old cachedData results even when calling createCachedData() after running the scripts (~800 modules). Does this sound likely or could this indicate something is slightly off?

Update:

NM, on double checking my test I found I goofed it.
The new API does produce more info in the buffers 😋

@hashseed hashseed referenced this pull request Jun 22, 2018

Closed

wip #1

@devsnek

This comment has been minimized.

Member

devsnek commented Jun 26, 2018

landed in 4f67c6f

@devsnek devsnek closed this Jun 26, 2018

@devsnek devsnek deleted the devsnek:feature/vm-snapshot-on-the-go-anytime-anywhere branch Jun 26, 2018

devsnek added a commit that referenced this pull request Jun 26, 2018

vm: add Script.createCodeCache()
PR-URL: #20300
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>

targos added a commit that referenced this pull request Jun 28, 2018

vm: add Script.createCodeCache()
PR-URL: #20300
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>

targos added a commit that referenced this pull request Jul 3, 2018

2018-07-04, Version 10.6.0 (Current)
Notable changes:

* build:
  * Node.js should now be about 60% faster to startup than the previous version,
    thanks to the use V8's code cache feature for core modules. [#21405](#21405)
* dns:
  * An experimental promisified version of the dns module is now available. Give
    it a try with `require('dns').promises`. [#21264](#21264)
* fs:
  * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498)
* lib:
  * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript
    specification ([reference](tc39/ecma262#1220)).
    Since Node.js now has experimental support for worker threads, we are being
    proactive and added a `notify` alias, while emitting a warning if
    `wake` is used. [#21413](#21413) [#21518](#21518)
* n-api:
  * Add API for asynchronous functions. [#17887](#17887)
* util:
  * `util.inspect` is now able to return a result instead of throwing when the
    maximum call stack size is exceeded during inspection. [#20725](#20725)
* vm:
  * Add `script.createCachedData()`. This API replaces the `produceCachedData`
    option of the `Script` constructor that is now deprecated. [#20300](#20300)
* worker:
  * Support for relative paths has been added to the `Worker` constructor. Paths
    are interpreted relative to the current working directory. [#21407](#21407)

@targos targos referenced this pull request Jul 3, 2018

Merged

v10.6.0 proposal #21629

targos added a commit that referenced this pull request Jul 3, 2018

2018-07-04, Version 10.6.0 (Current)
Notable changes:

* build:
  * Node.js should now be about 60% faster to startup than the previous version,
    thanks to the use V8's code cache feature for core modules. [#21405](#21405)
* dns:
  * An experimental promisified version of the dns module is now available. Give
    it a try with `require('dns').promises`. [#21264](#21264)
* fs:
  * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498)
* lib:
  * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript
    specification ([reference](tc39/ecma262#1220)).
    Since Node.js now has experimental support for worker threads, we are being
    proactive and added a `notify` alias, while emitting a warning if
    `wake` is used. [#21413](#21413) [#21518](#21518)
* n-api:
  * Add API for asynchronous functions. [#17887](#17887)
* util:
  * `util.inspect` is now able to return a result instead of throwing when the
    maximum call stack size is exceeded during inspection. [#20725](#20725)
* vm:
  * Add `script.createCachedData()`. This API replaces the `produceCachedData`
    option of the `Script` constructor that is now deprecated. [#20300](#20300)
* worker:
  * Support for relative paths has been added to the `Worker` constructor. Paths
    are interpreted relative to the current working directory. [#21407](#21407)

PR-URL: #21629

targos added a commit that referenced this pull request Jul 4, 2018

2018-07-04, Version 10.6.0 (Current)
Notable changes:

* dns:
  * An experimental promisified version of the dns module is now available. Give
    it a try with `require('dns').promises`. [#21264](#21264)
* fs:
  * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498)
* lib:
  * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript
    specification ([reference](tc39/ecma262#1220)).
    Since Node.js now has experimental support for worker threads, we are being
    proactive and added a `notify` alias, while emitting a warning if
    `wake` is used. [#21413](#21413) [#21518](#21518)
* n-api:
  * Add API for asynchronous functions. [#17887](#17887)
* util:
  * `util.inspect` is now able to return a result instead of throwing when the
    maximum call stack size is exceeded during inspection. [#20725](#20725)
* vm:
  * Add `script.createCachedData()`. This API replaces the `produceCachedData`
    option of the `Script` constructor that is now deprecated. [#20300](#20300)
* worker:
  * Support for relative paths has been added to the `Worker` constructor. Paths
    are interpreted relative to the current working directory. [#21407](#21407)

PR-URL: #21629

targos added a commit that referenced this pull request Jul 4, 2018

2018-07-04, Version 10.6.0 (Current)
Notable changes:

* dns:
  * An experimental promisified version of the dns module is now available. Give
    it a try with `require('dns').promises`. [#21264](#21264)
* fs:
  * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498)
* lib:
  * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript
    specification ([reference](tc39/ecma262#1220)).
    Since Node.js now has experimental support for worker threads, we are being
    proactive and added a `notify` alias, while emitting a warning if
    `wake` is used. [#21413](#21413) [#21518](#21518)
* n-api:
  * Add API for asynchronous functions. [#17887](#17887)
* util:
  * `util.inspect` is now able to return a result instead of throwing when the
    maximum call stack size is exceeded during inspection. [#20725](#20725)
* vm:
  * Add `script.createCachedData()`. This API replaces the `produceCachedData`
    option of the `Script` constructor that is now deprecated. [#20300](#20300)
* worker:
  * Support for relative paths has been added to the `Worker` constructor. Paths
    are interpreted relative to the current working directory. [#21407](#21407)

PR-URL: #21629

@devsnek devsnek removed the author ready label Jul 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment