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

Some tests assume that a single compilation is not cached #6258

Closed
jeisinger opened this issue Apr 18, 2016 · 4 comments
Closed

Some tests assume that a single compilation is not cached #6258

jeisinger opened this issue Apr 18, 2016 · 4 comments
Labels
test Issues and PRs related to the tests.

Comments

@jeisinger
Copy link
Contributor

I'd like to cache scripts immediately instead of recompiling the same script twice before caching it. This breaks two tests:

parallel/test-v8-flags assumes that running the same script twice will result in the script being compiled twice, specifically, it flips the --allow-natives-syntax between the two compilations. This could be fixed by using eval() also when testing execution in a different context.

Note that in general, V8 makes no guarantees about flags being changeable after the first isolate was created

The other test is test-vm-cached-data.js which creates a cache for a valid script, and then corrupts the cache and compiles the script again. But since the script is now already in the memory cache, the passed in cache is ignored. Not sure what to do about this one. It looks like this also relies on a pretty borderline case. If you corrupted the cache between the second and third compilation, it would already get ignored now.

Sample output here: https://build.chromium.org/p/client.v8.fyi/builders/V8%20-%20node.js%20integration%20-%20lkgr/builds/243/steps/build%20and%20test%20node.js/logs/stdio

/cc @nodejs/v8

@mscdex mscdex added the test Issues and PRs related to the tests. label Apr 18, 2016
@ofrobots
Copy link
Contributor

I wonder if the v8.setFlagsFromString API even makes sense in Node.js. Here's the current documentation:

Set additional V8 command line flags. Use with care; changing settings after the VM has started may result in unpredictable behavior, including crashes and data loss. Or it may simply do nothing.

@ChALkeR do you know if this gets much use by user-space modules on npm?

/cc @indutny for test-vm-cache-data.

@bnoordhuis
Copy link
Member

We use it quite a bit within StrongLoop (and yes, we're well aware of the gotchas - I wrote the code and the docs.)

@bnoordhuis
Copy link
Member

parallel/test-v8-flags assumes that running the same script twice will result in the script being compiled twice, specifically, it flips the --allow-natives-syntax between the two compilations. This could be fixed by using eval() also when testing execution in a different context.

I'm not 100% sure how to parse that sentence. The test executes eval('%_IsSmi(42)') and vm.runInThisContext('%_IsSmi(42)') twice. Is the idea to have a cache keyed on the source string so that the second invocation produces the exact same (identity-wise) v8::Script?

@jeisinger
Copy link
Contributor Author

eval and runInThisContext hit different caches. The cache currently works like this: the first time we compile something, we put a marker in the cache, but not the actual script. On the second compilation, we replace the marker with the actual script, so the third and future attempts to compile will just get the entry from the cache.

This was introduced for eval() on the assumption that it's unlikely that you'll eval the same string twice, so in order to safe memory, we wait until you actually compile it twice before caching.

For scripts this makes less sense, as you might run in several times in different contexts (or iframes in a browser). Anyways,

so if you changed the test to call eval() twice before flipping the flag, it would also fail.

indutny added a commit to indutny/io.js that referenced this issue Apr 19, 2016
V8 may start caching scripts from the first run soon. Preventively
execute scripts in another process to ensure no test failures due to
an update in the future.

See: nodejs#6258
indutny added a commit that referenced this issue Apr 20, 2016
V8 may start caching scripts from the first run soon. Preventively
execute scripts in another process to ensure no test failures due to
an update in the future.

See: #6258
PR-URL: #6280
Reviewed-By: Jochen Eisinger <jochen@chromium.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
ofrobots added a commit to ofrobots/node that referenced this issue Apr 20, 2016
V8 may cache compiled scripts, and it is not safe to assume that the
V8 flags can be changed after an isolate has been created. In this
particular case, since we are using the same script multiple times,
the test would fail if V8 cached the result of the compilation.

Ref: nodejs#6280
Fixes: nodejs#6258
joelostrowski pushed a commit to joelostrowski/node that referenced this issue Apr 25, 2016
V8 may start caching scripts from the first run soon. Preventively
execute scripts in another process to ensure no test failures due to
an update in the future.

See: nodejs#6258
PR-URL: nodejs#6280
Reviewed-By: Jochen Eisinger <jochen@chromium.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
joelostrowski pushed a commit to joelostrowski/node that referenced this issue Apr 25, 2016
V8 may cache compiled scripts, and it is not safe to assume that the
V8 flags can be changed after an isolate has been created. In this
particular case, since we are using the same script multiple times,
the test would fail if V8 cached the result of the compilation.

Ref: nodejs#6280
Fixes: nodejs#6258
PR-URL: nodejs#6316
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
jasnell pushed a commit that referenced this issue Apr 26, 2016
V8 may start caching scripts from the first run soon. Preventively
execute scripts in another process to ensure no test failures due to
an update in the future.

See: #6258
PR-URL: #6280
Reviewed-By: Jochen Eisinger <jochen@chromium.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this issue Apr 26, 2016
V8 may cache compiled scripts, and it is not safe to assume that the
V8 flags can be changed after an isolate has been created. In this
particular case, since we are using the same script multiple times,
the test would fail if V8 cached the result of the compilation.

Ref: #6280
Fixes: #6258
PR-URL: #6316
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants