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

src: clean up ArrayBufferAllocator #7082

Merged
merged 4 commits into from Jun 1, 2016

Conversation

Projects
None yet
7 participants
@bnoordhuis
Member

bnoordhuis commented May 31, 2016

This is prep work for multi-isolate support. The main goal is to break the dependency between ArrayBufferAllocator and Environment, which is an unholy union that should never have been.

I think it's rather nice that it manages to do that while still reducing the line count by 90 lines.

R=@jasnell? I'd R=@trevnorris too but I think he is on holiday?

CI: https://ci.nodejs.org/job/node-test-pull-request/2881/

flags[kNoZeroFill] = 0;
}
if (noZeroFill)
zeroFill[0] = 0; // Reset by the runtime.

This comment has been minimized.

@trevnorris

trevnorris May 31, 2016

Contributor

Why drop kNoZeroFill?

This comment has been minimized.

@bnoordhuis

bnoordhuis May 31, 2016

Member

You mean the constant? No real reason except that it's a single-element array now.

@jasnell

This comment has been minimized.

Member

jasnell commented May 31, 2016

Generally LGTM. CI is green. Nothing sticks out immediately as a concern.

@trevnorris

View changes

src/node.cc Outdated
// without a context.
IsolateData isolate_data(isolate, instance_data->event_loop());

Environment* env = CreateEnvironment(&isolate_data,

This comment has been minimized.

@trevnorris

trevnorris May 31, 2016

Contributor

I could be totally off, but why not use direct initialization with Environment? Its lifetime can't exceed that of IsolateData.

This comment has been minimized.

@bnoordhuis

bnoordhuis May 31, 2016

Member

I confess I'm not sure what you mean by 'direct initialization'. Making Environment a member of IsolateData? There can be more than one Environment per isolate so that won't work.

Or do you mean making env a stack-allocated variable?

This comment has been minimized.

@trevnorris

trevnorris May 31, 2016

Contributor

@bnoordhuis making it a stack-allocated variable. basically doing:

Environment env(&isolate_data,

I just can't see a case where it would outlive the stack it's declared in.

EDIT: TBH Google helped me find the term about a minute before I posted this.

This comment has been minimized.

@trevnorris

trevnorris May 31, 2016

Contributor

ah, I see now that it's a private constructor. welp, no reason to change it if it's not broken.

@trevnorris

This comment has been minimized.

Contributor

trevnorris commented May 31, 2016

@bnoordhuis Just got back last night. Thanks for cleaning up that implementation. LGTM.

bnoordhuis added some commits May 31, 2016

src: move IsolateData out of Environment
A follow-up commit is going to make IsolateData creation explicit.
In order for that to work, it needs to move out of Environment.

PR-URL: #7082
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
src: make IsolateData creation explicit
Make it easier to reason about the lifetime and the ownership of the
IsolateData instance by making its creation explicit and by removing
reference counting logic.

The creator of the Environment is now responsible for passing in the
IsolateData instance and for keeping it alive as long as the Environment
is alive.

PR-URL: #7082
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
lib,src: drop dependency on v8::Private::ForApi()
Said function requires that a v8::Context has been entered first,
introducing a chicken-and-egg problem when creating the first context.

PR-URL: #7082
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
lib,src: clean up ArrayBufferAllocator
Remove the direct dependency on node::Environment (which is per-context)
from node::ArrayBufferAllocator (which is per-isolate.)

Contexts that want to toggle the zero fill flag, now do so through a
field that is owned by ArrayBufferAllocator.  Better, still not great.

PR-URL: #7082
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

@bnoordhuis bnoordhuis force-pushed the bnoordhuis:multi-isolate-prep-work branch to 27e84dd Jun 1, 2016

@bnoordhuis bnoordhuis closed this Jun 1, 2016

@bnoordhuis bnoordhuis deleted the bnoordhuis:multi-isolate-prep-work branch Jun 1, 2016

@bnoordhuis bnoordhuis merged commit 27e84dd into nodejs:master Jun 1, 2016

zeroFill[0] = 0; // Reset by the runtime.
const ui8 = new Uint8Array(size);
Object.setPrototypeOf(ui8, Buffer.prototype);
return ui8;

This comment has been minimized.

@ChALkeR

ChALkeR Jun 1, 2016

Member

Why was try-finally dropped here? It breaks things.

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Jun 1, 2016

Perhaps revert this and give it more review time?

ChALkeR added a commit to ChALkeR/io.js that referenced this pull request Jun 1, 2016

Revert "lib,src: clean up ArrayBufferAllocator"
This reverts commit 27e84dd.

nodejs#7082 was landed too fast and did
not have sufficient review time.

That PR also broke some things (testcases will follow separately).

ChALkeR added a commit to ChALkeR/io.js that referenced this pull request Jun 1, 2016

Revert "lib,src: drop dependency on v8::Private::ForApi()"
This reverts commit 334ef4f.

nodejs#7082 was landed too fast and did
not have sufficient review time.

That PR also broke some things (testcases will follow separately).

ChALkeR added a commit to ChALkeR/io.js that referenced this pull request Jun 1, 2016

Revert "src: make IsolateData creation explicit"
This reverts commit c3cd453.

nodejs#7082 was landed too fast and did
not have sufficient review time.

That PR also broke some things (testcases will follow separately).

ChALkeR added a commit to ChALkeR/io.js that referenced this pull request Jun 1, 2016

Revert "src: move IsolateData out of Environment"
This reverts commit 0301ce9.

nodejs#7082 was landed too fast and did
not have sufficient review time.

That PR also broke some things (testcases will follow separately).

@ChALkeR ChALkeR referenced this pull request Jun 1, 2016

Closed

Revert 7082 #7092

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Jun 1, 2016

Revert in #7092. Upd: closed.

@addaleax addaleax referenced this pull request Jun 1, 2016

Closed

buffer: improve creation performance #6893

2 of 2 tasks complete

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Jun 2, 2016

src: use stack-allocated Environment instances
Makes it easier to reason about the lifetime of the Environment object.

PR-URL: nodejs#7090
Refs: nodejs#7082
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

@addaleax addaleax referenced this pull request Jun 3, 2016

Closed

src: fix ArrayBuffer size for zero fill flag #7142

2 of 2 tasks complete
@trevnorris

This comment has been minimized.

Contributor

trevnorris commented on 334ef4f Jun 21, 2016

@bnoordhuis Does PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V) in src/node_util.cc give you an:

warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]

This comment has been minimized.

Member

bnoordhuis replied Jun 21, 2016

I don't see that warning locally, no.

This comment has been minimized.

Contributor

trevnorris replied Jun 21, 2016

hm. maybe it's because of ccache. thanks.

@Fishrock123 Fishrock123 referenced this pull request Jul 5, 2016

Closed

buffers: speed up swap16/32, add swap64 #7157

4 of 4 tasks complete
@trevnorris

This comment has been minimized.

Contributor

trevnorris commented on src/node_util.cc in 334ef4f Aug 29, 2016

Bothered that I didn't notice this before, but since this uses the Maybe API it should end with .FromJust(). Currently I'm getting a stack of warnings every time I compile.

@Trott

This comment has been minimized.

Member

Trott commented on lib/buffer.js in 27e84dd Sep 26, 2016

@bnoordhuis If we wanted to run a test that exercised the right hand side of the || below, how would we do that? Like, what does it mean to "not own the ArrayBuffer allocator"? Is this a test that can be performed in JS-land or not so much? /cc @oogz

This comment has been minimized.

Member

bnoordhuis replied Sep 27, 2016

You could do it through a cctest or add-on test that creates an isolate with a custom v8::ArrayBuffer::Allocator and a node context with node::CreateEnvironment().

This comment has been minimized.

Member

bengl replied Mar 1, 2017

I'm full of questions: Is the || [0] even necessary? Isn't it safe to assume that lib/buffer.js is always running with node's ArrayBuffer::Allocator? Is there a (realistic) environment is using node::CreateEnvironment() without node's allocator? Are such environments actually supported? Should they be?

This comment has been minimized.

Member

bnoordhuis replied Mar 2, 2017

node::CreateEnvironment() was added by popular demand from people embedding node so yes, I would say it is in use and no, they won't be using node's allocator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment