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

vm: fix produceCachedData #5343

Closed
wants to merge 2 commits into from
Closed

vm: fix produceCachedData #5343

wants to merge 2 commits into from

Conversation

jray319
Copy link
Contributor

@jray319 jray319 commented Feb 21, 2016

Fix segmentation faults when compiling the same code with
produceCachedData option. V8 ignores the option when the code is in its
compilation cache and does not return cached data. Added
cachedDataProduced property to v8.Script to denote whether the cached
data is produced successfully.

Fix segmentation faults when compiling the same code with
`produceCachedData` option. V8 ignores the option when the code is in its
compilation cache and does not return cached data. Added
`cachedDataProduced` property to `v8.Script` to denote whether the cached
data is produced successfully.
@mscdex mscdex added the vm Issues and PRs related to the vm subsystem. label Feb 21, 2016
@mscdex
Copy link
Contributor

mscdex commented Feb 21, 2016

/cc @indutny

reinterpret_cast<const char*>(cached_data->data),
cached_data->length);
args.This()->Set(env->cached_data_string(), buf.ToLocalChecked());
bool cached_data_produced = (cached_data != nullptr);
Copy link
Member

Choose a reason for hiding this comment

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

No need in parens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed. Thanks.

@indutny
Copy link
Member

indutny commented Feb 21, 2016

One nit, otherwise looking good. Going to give a try before landing, though.

@indutny
Copy link
Member

indutny commented Feb 21, 2016

@indutny
Copy link
Member

indutny commented Feb 21, 2016

LGTM

@indutny
Copy link
Member

indutny commented Feb 21, 2016

@jasnell this is not in v4.x, please remove the tag.

@MylesBorins
Copy link
Contributor

@indutny the watch tag is for us to audit a commit to be backported

@indutny
Copy link
Member

indutny commented Feb 21, 2016

@thealphanerd I'm just saying that it should be don't land on v4.x

@MylesBorins
Copy link
Contributor

Ahhh... got it 😄

@indutny
Copy link
Member

indutny commented Feb 21, 2016

Landed in d5c04c3, thank you!

@indutny indutny closed this Feb 21, 2016
indutny pushed a commit that referenced this pull request Feb 21, 2016
Fix segmentation faults when compiling the same code with
`produceCachedData` option. V8 ignores the option when the code is in
its compilation cache and does not return cached data. Added
`cachedDataProduced` property to `v8.Script` to denote whether the
cached data is produced successfully.

PR-URL: #5343
Reviewed-By: Fedor Indutny <[email protected]>
@indutny
Copy link
Member

indutny commented Feb 21, 2016

Changed tags myself.

@indutny
Copy link
Member

indutny commented Feb 21, 2016

Argh, concurrency! :)

@MylesBorins
Copy link
Contributor

@indutny would you be able to add a small sentence about why this commit is not an LTS candidate?

@indutny
Copy link
Member

indutny commented Feb 21, 2016

Because produceCachedData is a v5.x only thing.

@indutny
Copy link
Member

indutny commented Feb 21, 2016

@thealphanerd we may want to reconsider this a bit later, though. I just don't want to make a commitment to supporting this on a LTS level yet. If things will work out - there are no blockers in backporting all of code cache changes to v4.x and releasing them in next minor.

rvagg pushed a commit that referenced this pull request Feb 21, 2016
Fix segmentation faults when compiling the same code with
`produceCachedData` option. V8 ignores the option when the code is in
its compilation cache and does not return cached data. Added
`cachedDataProduced` property to `v8.Script` to denote whether the
cached data is produced successfully.

PR-URL: #5343
Reviewed-By: Fedor Indutny <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants