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

test: v8-flags is sensitive to script caching #6316

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
7 participants
@ofrobots
Copy link
Contributor

commented Apr 20, 2016

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

V8 may cache compiled scripts, and it is not safe to assume that the
V8 flags may 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.

This PR, along with #6280 is need to fix the Node.js integration builds that V8 is running against V8 tip-of-tree.

Ref: #6280
Fixes: #6258

R=@bnoordhuis @jeisinger
CI: https://ci.nodejs.org/job/node-test-pull-request/2349/

test: v8-flags is sensitive to script caching
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

@ofrobots ofrobots force-pushed the ofrobots:test-v8-flags branch to d5caa00 Apr 20, 2016

@jasnell jasnell force-pushed the nodejs:master branch to 7e9b0dd Apr 20, 2016

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Apr 21, 2016

LGTM

3 similar comments
@targos

This comment has been minimized.

Copy link
Member

commented Apr 21, 2016

LGTM

@jeisinger

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2016

LGTM

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2016

LGTM

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 21, 2016

@ofrobots ... do you wanna get this one landed today so I can get it into the v6 RC.4?

ofrobots added a commit that referenced this pull request Apr 21, 2016

test: v8-flags is sensitive to script caching
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>
@ofrobots

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2016

Thanks. Landed as a770a16.

@ofrobots ofrobots closed this Apr 21, 2016

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 21, 2016

btw, would this be applicable to v8 v4.6 in the LTS branch also?

@ofrobots

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2016

I don't think this fix is necessary on 5.x and 4.x. The behaviour of V8 (especially w.r.t. compiled script caching) is fixed on those branches, and backporting this doesn't add too much value. That said, there is no harm in backporting this particular change.

joelostrowski added a commit to joelostrowski/node that referenced this pull request Apr 25, 2016

test: v8-flags is sensitive to script caching
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 added a commit that referenced this pull request Apr 26, 2016

test: v8-flags is sensitive to script caching
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
You can’t perform that action at this time.