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

src: clean up ArrayBufferAllocator #7082

Merged
merged 4 commits into from
Jun 1, 2016

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis 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/

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. util Issues and PRs related to the built-in util module. c++ Issues and PRs that require attention from people who are familiar with C++. labels May 31, 2016
flags[kNoZeroFill] = 0;
}
if (noZeroFill)
zeroFill[0] = 0; // Reset by the runtime.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why drop kNoZeroFill?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@jasnell
Copy link
Member

jasnell commented May 31, 2016

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

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

Environment* env = CreateEnvironment(&isolate_data,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@trevnorris trevnorris May 31, 2016

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@trevnorris
Copy link
Contributor

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

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: nodejs#7082
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
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: nodejs#7082
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Said function requires that a v8::Context has been entered first,
introducing a chicken-and-egg problem when creating the first context.

PR-URL: nodejs#7082
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
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: nodejs#7082
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
@bnoordhuis bnoordhuis closed this Jun 1, 2016
@bnoordhuis bnoordhuis deleted the multi-isolate-prep-work branch June 1, 2016 07:35
@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;
Copy link
Member

@ChALkeR ChALkeR Jun 1, 2016

Choose a reason for hiding this comment

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

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

@ChALkeR
Copy link
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
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
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
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
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 mentioned this pull request Jun 1, 2016
@ChALkeR
Copy link
Member

ChALkeR commented Jun 1, 2016

Revert in #7092. Upd: closed.

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Jun 2, 2016
Makes it easier to reason about the lifetime of the Environment object.

PR-URL: nodejs#7090
Refs: nodejs#7082
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants