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

contextify: use offset/length from Uint8Array #4947

Closed
wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Jan 29, 2016

Do not blindly take data from underlying ArrayBuffer, use
ByteOffset/ByteLength of Uint8Array itself.

Additionally, fix tests that weren't actually properly running because
of V8's internal code cache. The code should be different, otherwise the
cached data won't be used at all.

Fix: #4939

R = @nodejs/v8 or @trevnorris

Do not blindly take data from underlying `ArrayBuffer`, use
`ByteOffset`/`ByteLength` of `Uint8Array` itself.

Additionally, fix tests that weren't actually properly running because
of V8's internal code cache. The code should be different, otherwise the
cached data won't be used at all.

Fix: nodejs#4939
@indutny
Copy link
Member Author

indutny commented Jan 29, 2016

@mscdex mscdex added vm Issues and PRs related to the vm subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Jan 29, 2016
@@ -4,30 +4,58 @@ const assert = require('assert');
const vm = require('vm');
const Buffer = require('buffer').Buffer;

const originalSource = '(function bcd() { return \'original\'; })';
function getSource(tag) {
return '(function ' + tag + '() { return \'' + tag + '\'; })';
Copy link
Member

Choose a reason for hiding this comment

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

Template string?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.

@bnoordhuis
Copy link
Member

LGTM with a suggestion.

@indutny
Copy link
Member Author

indutny commented Jan 29, 2016

Landed in b4ece1b, thank you!

@indutny indutny closed this Jan 29, 2016
indutny added a commit that referenced this pull request Jan 29, 2016
Do not blindly take data from underlying `ArrayBuffer`, use
`ByteOffset`/`ByteLength` of `Uint8Array` itself.

Additionally, fix tests that weren't actually properly running because
of V8's internal code cache. The code should be different, otherwise the
cached data won't be used at all.

Fix: #4939
PR-URL: #4947
Reviewed-By: Ben Noordhuis <[email protected]>
@jasnell
Copy link
Member

jasnell commented Feb 1, 2016

lts-watch label applied

@indutny indutny deleted the fix/gh-4939 branch February 1, 2016 19:13
@indutny
Copy link
Member Author

indutny commented Feb 1, 2016

@jasnell I don't think that there is a point, this patch fixes bug in new API introduced recently in 96934cb

rvagg pushed a commit that referenced this pull request Feb 10, 2016
Do not blindly take data from underlying `ArrayBuffer`, use
`ByteOffset`/`ByteLength` of `Uint8Array` itself.

Additionally, fix tests that weren't actually properly running because
of V8's internal code cache. The code should be different, otherwise the
cached data won't be used at all.

Fix: #4939
PR-URL: #4947
Reviewed-By: Ben Noordhuis <[email protected]>
@MylesBorins
Copy link
Contributor

dropping LTS label based on what @indutny said.

Feel free to update

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Do not blindly take data from underlying `ArrayBuffer`, use
`ByteOffset`/`ByteLength` of `Uint8Array` itself.

Additionally, fix tests that weren't actually properly running because
of V8's internal code cache. The code should be different, otherwise the
cached data won't be used at all.

Fix: nodejs#4939
PR-URL: nodejs#4947
Reviewed-By: Ben Noordhuis <[email protected]>
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++. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code cache is rejected if it's size is under about 8K
5 participants