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
base: master
from

Conversation

Projects
None yet
7 participants
@zertosh
Contributor

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

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Feb 23, 2017

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

Member

bnoordhuis commented Feb 23, 2017

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

This comment has been minimized.

Show comment
Hide comment
@zertosh

zertosh Feb 23, 2017

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 23, 2017

Member

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)

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

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Feb 23, 2017

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.

Member

addaleax commented Feb 23, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 23, 2017

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Feb 23, 2017

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

Member

addaleax commented Feb 23, 2017

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

This comment has been minimized.

Show comment
Hide comment
@zertosh

zertosh Feb 23, 2017

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Feb 23, 2017

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(),
Member

bnoordhuis commented Feb 23, 2017

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

This comment has been minimized.

Show comment
Hide comment
@zertosh

zertosh Feb 23, 2017

Contributor

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

Contributor

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.

Show outdated Hide outdated doc/api/vm.md
@@ -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

This comment has been minimized.

@addaleax

addaleax Feb 25, 2017

Member

Please use added: REPLACEME

@addaleax

addaleax Feb 25, 2017

Member

Please use added: REPLACEME

Show outdated Hide outdated doc/api/vm.md
-->
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.

This comment has been minimized.

@addaleax

addaleax Feb 25, 2017

Member

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

@addaleax

addaleax Feb 25, 2017

Member

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

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Feb 27, 2017

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.

Member

bnoordhuis commented Feb 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@zertosh

zertosh Mar 2, 2017

Contributor

@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();
}
Contributor

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

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Mar 2, 2017

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.

Member

bnoordhuis commented Mar 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 2, 2017

Member

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

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.

zertosh added some commits Feb 23, 2017

vm: add cachedDataVersionTag
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 from vm: add cachedDataVersionTag to v8: add cachedDataVersionTag Mar 3, 2017

@zertosh

This comment has been minimized.

Show comment
Hide comment
@zertosh

zertosh Mar 3, 2017

Contributor

hmmm. why aren't the CI tests running?

Contributor

zertosh commented Mar 3, 2017

hmmm. why aren't the CI tests running?

@bnoordhuis

Because it needs to be started manually. :-)

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

Show outdated Hide outdated doc/api/v8.md
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.

This comment has been minimized.

@bnoordhuis

bnoordhuis Mar 3, 2017

Member

I'd s/another V8/this instance of V8/ to emphasize that it applies to the running instance of V8.

@bnoordhuis

bnoordhuis Mar 3, 2017

Member

I'd s/another V8/this instance of V8/ to emphasize that it applies to the running instance of V8.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Mar 5, 2017

Member

Landed in 70beef9, thanks for the PR! 🎉

Member

addaleax commented Mar 5, 2017

Landed in 70beef9, thanks for the PR! 🎉

@addaleax addaleax closed this Mar 5, 2017

addaleax added a commit that referenced this pull request Mar 5, 2017

v8: add cachedDataVersionTag
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 <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@evanlucas

This comment has been minimized.

Show comment
Hide comment
@evanlucas

evanlucas Mar 7, 2017

Member

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

Member

evanlucas commented Mar 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@zertosh

zertosh Mar 7, 2017

Contributor

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())));
}

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@zertosh

zertosh Mar 7, 2017

Contributor

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.

Contributor

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