Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upvm: add Script.createCodeCache() #20300
Conversation
devsnek
requested a review
from
hashseed
Apr 25, 2018
nodejs-github-bot
added
the
lib / src
label
Apr 25, 2018
devsnek
added
vm
and removed
lib / src
labels
Apr 25, 2018
addaleax
reviewed
Apr 25, 2018
| if (cachedData && !cachedDataWarned) { | ||
| process.emitWarning(cachedDataMessage, 'DeprecationWarning', 'DEPXXXX'); | ||
| cachedDataWarned = true; | ||
| } |
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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 :-)
addaleax
added
the
semver-major
label
Apr 25, 2018
devsnek
added
semver-minor
and removed
semver-major
labels
Apr 25, 2018
devsnek
force-pushed the
devsnek:feature/vm-snapshot-on-the-go-anytime-anywhere
branch
from
0a65658
to
d4aa733
Apr 25, 2018
hashseed
reviewed
Apr 26, 2018
| Type: Documentation-only | ||
| The options `produceCachedData` and `cachedData` have been deprecated. Please use |
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
devsnek
Apr 26, 2018
Author
Member
I changed it to docs only, not sure why I made it runtime in the first place
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment was marked as resolved.
hashseed
Apr 26, 2018
Member
I think you need to delete cache_data. Or even better, use unique_ptr around it.
jdalton
referenced this pull request
Apr 26, 2018
Open
vm module should expose V8::ScriptCompiler::CreateCodeCache #20052
hashseed
reviewed
Apr 26, 2018
| 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.
This comment has been minimized.
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.
TimothyGu
reviewed
Apr 26, 2018
| * `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.
This comment has been minimized.
TimothyGu
Apr 26, 2018
Member
Is the code cache generated by createCodeCache compatible with cachedData
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
This comment has been minimized.
|
@hashseed Can the code cache generated by a different version of v8 be loaded by another? (My guess is no?) |
This comment has been minimized.
This comment has been minimized.
|
We check a version hash before deserializing. If the version hash mismatches, V8 rejects the cache data. |
This comment has been minimized.
This comment has been minimized.
|
@hashseed Thanks. It would be good to document about this (otherwise the user only has a boolean codeCacheRejected without further information or possible cause) |
This comment has been minimized.
This comment has been minimized.
|
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 |
This comment has been minimized.
This comment has been minimized.
|
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. |
devsnek
force-pushed the
devsnek:feature/vm-snapshot-on-the-go-anytime-anywhere
branch
2 times, most recently
from
f066d12
to
b45e969
Apr 28, 2018
| --> | ||
|
|
||
| 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.
This comment was marked as resolved.
| - 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.
This comment was marked as resolved.
|
|
||
| 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.
This comment has been minimized.
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.
This comment has been minimized.
| if (script.cachedDataProduced) | ||
| data = script.cachedData.toString('base64'); |
This comment has been minimized.
This comment has been minimized.
| <a id="DEP00XX"></a> | ||
| ### DEP00XX: vm.Script cached data | ||
| Type: Documentation-only |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
devsnek
Apr 28, 2018
Author
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.
This comment has been minimized.
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.
This comment has been minimized.
devsnek
Apr 28, 2018
Author
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.
This comment has been minimized.
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. :)
This comment has been minimized.
This comment has been minimized.
|
These here are possible reject reasons. |
This comment has been minimized.
This comment has been minimized.
|
@hashseed I meant possible reasons for failure of creating a code cache, not consuming one :) |
This comment has been minimized.
This comment has been minimized.
|
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. |
This comment has been minimized.
This comment has been minimized.
|
Any updates here? What's the status? |
This comment has been minimized.
This comment has been minimized.
|
@jdalton hashseed is going to backport a bunch of changes to SharedFunctionInfo soonish, this is just waiting for that. |
This comment has been minimized.
This comment has been minimized.
|
Umm.. wait. Which changes am I supposed to backport? I am not aware of anything actionable on my end. Did I drop the ball? |
This comment has been minimized.
This comment has been minimized.
|
@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 |
This comment has been minimized.
This comment has been minimized.
|
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? |
This comment has been minimized.
This comment has been minimized.
|
@devsnek Are you cool floating the change into Node? |
devsnek
force-pushed the
devsnek:feature/vm-snapshot-on-the-go-anytime-anywhere
branch
from
ab4284b
to
c31b520
Jun 19, 2018
This comment has been minimized.
This comment has been minimized.
|
Okay! |
This comment has been minimized.
This comment has been minimized.
jasnell
approved these changes
Jun 21, 2018
jdalton
approved these changes
Jun 21, 2018
TimothyGu
reviewed
Jun 21, 2018
| assert(cachedData instanceof Buffer); | ||
| data = cachedData.toString('base64'); |
This comment has been minimized.
This comment has been minimized.
TimothyGu
Jun 21, 2018
Member
This overwrites data created earlier. Is it possible to test the buffer returned from both produceCachedData and createCachedData?
This comment was marked as outdated.
This comment was marked as outdated.
|
The failing tests look unrelated. On the travis they point to:
On jenkins they point to:
and
|
devsnek
force-pushed the
devsnek:feature/vm-snapshot-on-the-go-anytime-anywhere
branch
from
c31b520
to
9db4f30
Jun 21, 2018
This comment has been minimized.
This comment has been minimized.
TimothyGu
approved these changes
Jun 22, 2018
addaleax
approved these changes
Jun 22, 2018
addaleax
added
the
C++
label
Jun 22, 2018
This comment has been minimized.
This comment has been minimized.
|
CI again just to make sure: https://ci.nodejs.org/job/node-test-commit/19379/ |
addaleax
added
the
author ready
label
Jun 22, 2018
hashseed
approved these changes
Jun 22, 2018
This comment has been minimized.
This comment has been minimized.
|
Update: NM, on double checking my test I found I goofed it. |
This comment has been minimized.
This comment has been minimized.
|
landed in 4f67c6f |
devsnek commentedApr 25, 2018
•
edited
Closes #20052
/cc @nodejs/vm
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes