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

v8: add cachedDataVersionTag #11515

Closed
wants to merge 3 commits into from
Closed

v8: add cachedDataVersionTag #11515

wants to merge 3 commits into from

Conversation

zertosh
Copy link
Contributor

@zertosh zertosh commented Feb 23, 2017

Adds v8.cachedDataVersionTag(), which returns an integer representing the version tag for cachedData for the current V8 version & flags. Closes #11506.

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
Affected core subsystem(s)

v8

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem. labels Feb 23, 2017
@bnoordhuis
Copy link
Member

I have two issues with this approach:

  1. It's tied a little too much to a particular V8 API that hasn't been stable over time.

  2. 'Version' is going to mislead people into thinking it's related to the node.js or V8 version when it's not. It's derived from the command line flags and detected CPU features, both of which can change between invocations and even at runtime with v8.setFlagsFromString().

@zertosh
Copy link
Contributor Author

zertosh commented Feb 23, 2017

@bnoordhuis, very legitimate points.

  1. I don't know what other approach to take, but I'm very open to suggestions/help (this is my first time writing C++).

  2. I don't feel strongly about the name, or where it's exposed from. Do you think it makes more sense to expose it on process or maybe even v8? What about calling it getInternalCachedDataTag? Should I maybe also expand on the documentation to explain that the value can change at runtime?

@jasnell
Copy link
Member

jasnell commented Feb 23, 2017

Given the very v8 specific nature of this, would it make sense to add an API to the v8 module instead? something like require('v8').getCachedDataVersionTag(script)

@addaleax
Copy link
Member

@jasnell We could do that but it would be weird having it on a completely different module than where the actual caching occurs.

We could probably reserve some specific value (undefined? :D) to indicate “this value is not available”, and explicitly document that, even if it would not currently be produced by Node. That might also make things easier for node-chakracore.

@jasnell
Copy link
Member

jasnell commented Feb 23, 2017

Still seems a bit troublesome. I'd really not expose something new on vm that really only has utility with V8, even if we can return undefined for everything else

@addaleax
Copy link
Member

I'd really not expose something new on vm that really only has utility with V8, even if we can return undefined for everything else

I feel the same way, the thing is that that ship has kind of sailed when we added the cached data stuff in the first place. ;)

@zertosh
Copy link
Contributor Author

zertosh commented Feb 23, 2017

@jasnell and @addaleax, I see the argument for both sides, so I'll wait until someone gives me a definitive direction in which to take this PR. Given that the cached data feature is so obscure, I don't really feel strongly about where the version tag stuff lives.

@bnoordhuis
Copy link
Member

Perhaps we can add a property with a neutral-ish name to the result object? node-chakra could implement that by omitting it or setting it to zero.

diff --git a/src/env.h b/src/env.h
index 581d7e9..5e1ee13 100644
--- a/src/env.h
+++ b/src/env.h
@@ -74,9 +74,10 @@ namespace node {
   V(bytes_string, "bytes")                                                    \
   V(bytes_parsed_string, "bytesParsed")                                       \
   V(bytes_read_string, "bytesRead")                                           \
-  V(cached_data_string, "cachedData")                                         \
   V(cached_data_produced_string, "cachedDataProduced")                        \
   V(cached_data_rejected_string, "cachedDataRejected")                        \
+  V(cached_data_string, "cachedData")                                         \
+  V(cached_data_tag_string, "cachedDataTag")                                  \
   V(callback_string, "callback")                                              \
   V(change_string, "change")                                                  \
   V(oncertcb_string, "oncertcb")                                              \
diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index 9b0ab4e..5a08347 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -573,6 +573,10 @@ class ContextifyScript : public BaseObject {
             reinterpret_cast<const char*>(cached_data->data),
             cached_data->length);
         args.This()->Set(env->cached_data_string(), buf.ToLocalChecked());
+        args.This()->Set(
+            env->cached_data_tag_string(),
+            Integer::NewFromUnsigned(env->isolate(),
+                                     ScriptCompiler::CachedDataVersionTag()));
       }
       args.This()->Set(
           env->cached_data_produced_string(),

@zertosh
Copy link
Contributor Author

zertosh commented Feb 23, 2017

@bnoordhuis the reason I wanted it as a method was to be able to know before doing expensive IO loading a cached value if it's likely to work or not.

doc/api/vm.md Outdated
@@ -22,6 +22,14 @@ added: v0.3.1
Instances of the `vm.Script` class contain precompiled scripts that can be
executed in specific sandboxes (or "contexts").

## vm.Script.cachedDataVersionTag()
<!-- YAML
added: v7.x.x
Copy link
Member

Choose a reason for hiding this comment

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

Please use added: REPLACEME

doc/api/vm.md Outdated
-->

Returns an integer representing the version tag for `cachedData` for the current V8 version & flags.
* This value is meant only for determining whether a previously generated `cachedData` buffer is still valid; the tag has no other meaning.
Copy link
Member

Choose a reason for hiding this comment

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

This doesn’t really need to be a list, does it? (Also, we prefer line wrapping at 80 characters)

@bnoordhuis
Copy link
Member

the reason I wanted it as a method was to be able to know before doing expensive IO loading a cached value if it's likely to work or not

Can you go into more detail on your use case? I'm trying to understand how and where you'd use the tag.

@zertosh
Copy link
Contributor Author

zertosh commented Mar 2, 2017

@bnoordhuis, sure, in v8-compile-cache I store all of the cached data in a single file. That turns out to be a lot faster, than to load lots of small individual files of cached data as needed. But this means that if the cache data doesn't match your v8+cpu+flags, then you loaded that for nothing. I'm deploying this across a fleet of mostly homogenous machines, which means that I can pre-generate the cached data and ship it with the code. However, if a machine is one of the different ones, I'd like to be able to know that my cache won't work early on instead of waiting for a rejection.

If I had vm.Script.cachedDataVersionTag, I'd basically do:

const cachePath = path.join(CACHE_PATH, vm.Script.cachedDataVersionTag() + '.MAP');
if (fs.existsSync(cachePath)) {
  loadExistingCache(cachePath);
} else {
  createNewCache();
}

@bnoordhuis
Copy link
Member

Okay, thanks. I think a v8.cachedDataVersionTag() is the way to go in that case. It makes it clear it's V8-specific (which also means people can just go read the V8 documentation if they're confused by the 'Version' part) and we wouldn't have to agonize over backwards compatibility if upstream removes it.

James suggested passing the script object as an argument but I don't think that's necessary, the return value is independent of the script object.

@jasnell
Copy link
Member

jasnell commented Mar 2, 2017

Yep, I had misread the original PR. Agree that having it sit off v8 is the right thing to do.

Adds `vm.Script.cachedDataVersionTag()`, which returns an integer
representing the version tag for `cachedData` for the current V8
version & flags.
@zertosh zertosh changed the title vm: add cachedDataVersionTag v8: add cachedDataVersionTag Mar 3, 2017
@zertosh
Copy link
Contributor Author

zertosh commented Mar 3, 2017

hmmm. why aren't the CI tests running?

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Because it needs to be started manually. :-)

LGTM with a suggestion. CI: https://ci.nodejs.org/job/node-test-pull-request/6680/

doc/api/v8.md Outdated

Returns an integer representing a "version tag" derived from the V8 version,
command line flags and detected CPU features. This is useful for determining
whether a [`vm.Script`][] `cachedData` buffer is compatible with another V8.
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 s/another V8/this instance of V8/ to emphasize that it applies to the running instance of V8.

@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 5, 2017
@addaleax
Copy link
Member

addaleax commented Mar 5, 2017

Landed in 70beef9, thanks for the PR! 🎉

@addaleax addaleax closed this Mar 5, 2017
addaleax pushed a commit that referenced this pull request Mar 5, 2017
Adds `v8.cachedDataVersionTag()`, which returns an integer
representing the version tag for `cachedData` for the current V8
version & flags.

PR-URL: #11515
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@evanlucas
Copy link
Contributor

The test provided in this PR is failing when this change is cherry-picked v7.x-staging.

assert.js:81
  throw new assert.AssertionError({
  ^
AssertionError: 3483820116 !== 3483820116
    at Object.<anonymous> (/Users/evan/dev/code/forks/WORK-node/test/parallel/test-v8-version-tag.js:20:8)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.runMain (module.js:605:10)
    at run (bootstrap_node.js:425:7)
    at startup (bootstrap_node.js:146:9)
    at bootstrap_node.js:540:3

I wonder if the value being derived from the command line flags is a change that only exists in a more recent version of v8? /cc @nodejs/v8

For now, I'm going to hold off landing in v7.x

@zertosh
Copy link
Contributor Author

zertosh commented Mar 7, 2017

Weird, v7.x-staging's CachedDataVersionTag (

node/deps/v8/src/api.cc

Lines 2300 to 2304 in fdb4a6c

uint32_t ScriptCompiler::CachedDataVersionTag() {
return static_cast<uint32_t>(base::hash_combine(
internal::Version::Hash(), internal::FlagList::Hash(),
static_cast<uint32_t>(internal::CpuFeatures::SupportedFeatures())));
}
) includes the flags, just like current master

node/deps/v8/src/api.cc

Lines 2356 to 2360 in efaab8f

uint32_t ScriptCompiler::CachedDataVersionTag() {
return static_cast<uint32_t>(base::hash_combine(
internal::Version::Hash(), internal::FlagList::Hash(),
static_cast<uint32_t>(internal::CpuFeatures::SupportedFeatures())));
}

@zertosh
Copy link
Contributor Author

zertosh commented Mar 7, 2017

Ah but SetFlagsFromString is different 7a77daf#diff-914a24037ebfe44cbfab4f4ca7ff117aR672. I added the change to SetFlagsFromString from #10992 onto a cherry-pick of this PR on v7.x-staging, and the test then passes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. semver-minor PRs that contain new features and should be released in the next minor version. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants