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.

Sign up for GitHub

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

Jump to bottom

worker: make MessagePort uv_async_t inline field #26271

Closed
wants to merge 2 commits into from
Closed

worker: make MessagePort uv_async_t inline field #26271

wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

It’s not obvious why this was a heap allocation in the first place,
but it’s unneccessary. Most other HandleWraps also store the
libuv handle directly.

(This is going to have a minor merge conflict with #26099 so I’d wait until after that one lands before merging this.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@addaleax sadly an error occured when I tried to trigger a build :(

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 23, 2019
@addaleax addaleax added the worker Issues and PRs related to Worker support. label Feb 23, 2019
gireeshpunathil
gireeshpunathil reviewed Feb 23, 2019
View reviewed changes
src/node_messaging.cc
AsyncWrap::PROVIDER_MESSAGEPORT),
data_(new MessagePortData(this)) {
auto onmessage = [](uv_async_t* handle) {
// Called when data has been put into the queue.
MessagePort* channel = static_cast<MessagePort*>(handle->data);
MessagePort* channel = ContainerOf(&MessagePort::async_, handle);
Copy link
Member

Choose a reason for hiding this comment

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

I wanted to ask this earlier as well - what is the consideration behind obtaining this through pointer arithmetic over the other one?

Copy link
Member

Choose a reason for hiding this comment

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

For one, the address can't change; the contents of handle->data can.

In fact, the old code is mildly wrong because HandleWrap also sets handle_->data = this (where this is an instance of HandleWrap rather than MessagePort) so static_casting that to MessagePort afterwards would be wrong if multiple inheritance were involved.

src/node_messaging.cc
@@ -472,21 +472,13 @@ MessagePort::MessagePort(Environment* env,
Debug(this, "Created message port");
}

void MessagePort::AddToIncomingQueue(Message&& message) {
data_->AddToIncomingQueue(std::move(message));
}
Copy link
Member

Choose a reason for hiding this comment

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

how is this change related?

Copy link
Member Author

Choose a reason for hiding this comment

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

It’s not, it’s just something that I noticed, and putting it into another PR would have lead to yet another merge conflict. I’ve split it into a separate commit, as @bnoordhuis suggested.

Copy link
Member

Choose a reason for hiding this comment

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

thanks. part of my question (which I meant to ask but did not come out like that) was why this change required at all - that is not covered in the PR description. But I got it now as: it is an unused piece of code.

bnoordhuis
bnoordhuis approved these changes Feb 23, 2019
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

The removal of MessagePort::AddToIncomingQueue() should arguably be a separate commit.

src/node_messaging.cc
AsyncWrap::PROVIDER_MESSAGEPORT),
data_(new MessagePortData(this)) {
auto onmessage = [](uv_async_t* handle) {
// Called when data has been put into the queue.
MessagePort* channel = static_cast<MessagePort*>(handle->data);
MessagePort* channel = ContainerOf(&MessagePort::async_, handle);
Copy link
Member

Choose a reason for hiding this comment

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

For one, the address can't change; the contents of handle->data can.

In fact, the old code is mildly wrong because HandleWrap also sets handle_->data = this (where this is an instance of HandleWrap rather than MessagePort) so static_casting that to MessagePort afterwards would be wrong if multiple inheritance were involved.

@addaleax addaleax force-pushed the messageport-async-inline branch from d42e6e2 to be33b22 Compare February 23, 2019 19:30
addaleax added 2 commits March 1, 2019 10:15
@addaleax
worker: remove MessagePort::AddToIncomingQueue
e463971
@addaleax
worker: make MessagePort uv_async_t inline field
7c8f4c2 
It’s not obvious why this was a heap allocation in the first place,
but it’s unneccessary. Most other `HandleWrap`s also store the
libuv handle directly.
@addaleax addaleax force-pushed the messageport-async-inline branch from be33b22 to 7c8f4c2 Compare March 1, 2019 09:22
@addaleax
Copy link
Member Author

addaleax commented Mar 1, 2019

Rebased, new CI: https://ci.nodejs.org/job/node-test-pull-request/21081/

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 1, 2019
@addaleax
Copy link
Member Author

addaleax commented Mar 1, 2019

Landed in 4dd7c4b, 0fc40c8

@addaleax addaleax closed this Mar 1, 2019
@addaleax addaleax deleted the messageport-async-inline branch March 1, 2019 20:46
addaleax added a commit that referenced this pull request Mar 1, 2019
@addaleax
worker: remove MessagePort::AddToIncomingQueue
4dd7c4b 
PR-URL: #26271
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax added a commit that referenced this pull request Mar 1, 2019
@addaleax
worker: make MessagePort uv_async_t inline field
0fc40c8 
It’s not obvious why this was a heap allocation in the first place,
but it’s unneccessary. Most other `HandleWrap`s also store the
libuv handle directly.

PR-URL: #26271
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax added a commit that referenced this pull request Mar 1, 2019
@addaleax
worker: remove MessagePort::AddToIncomingQueue
51f01aa 
PR-URL: #26271
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax added a commit that referenced this pull request Mar 1, 2019
@addaleax
worker: make MessagePort uv_async_t inline field
6fdc502 
It’s not obvious why this was a heap allocation in the first place,
but it’s unneccessary. Most other `HandleWrap`s also store the
libuv handle directly.

PR-URL: #26271
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@BridgeAR BridgeAR mentioned this pull request Mar 4, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@richardlau richardlau richardlau approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. worker Issues and PRs related to Worker support.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@addaleax @nodejs-github-bot @bnoordhuis @jasnell @richardlau @gireeshpunathil