Skip to content

worker: make MessagePort uv_async_t inline field #26271

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

Closed
addaleax wants to merge 2 commits into from
Closed

worker: make MessagePort uv_async_t inline field #26271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e463971
worker: remove MessagePort::AddToIncomingQueue
addaleax Feb 23, 2019
7c8f4c2
worker: make MessagePort `uv_async_t` inline field
addaleax Feb 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 5 additions & 14 deletions src/node_messaging.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,18 +469,18 @@ MessagePort::MessagePort(Environment* env,
Local<Object> wrap)
: HandleWrap(env,
wrap,
reinterpret_cast<uv_handle_t*>(new uv_async_t()),
reinterpret_cast<uv_handle_t*>(&async_),
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

@gireeshpunathil gireeshpunathil Feb 23, 2019

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

@bnoordhuis bnoordhuis Feb 23, 2019

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.

channel->OnMessage();
};
CHECK_EQ(uv_async_init(env->event_loop(),
async(),
&async_,
onmessage), 0);
async()->data = static_cast<void*>(this);
async_.data = static_cast<void*>(this);

Local<Value> fn;
if (!wrap->Get(context, env->oninit_symbol()).ToLocal(&fn))
Expand All @@ -494,21 +494,13 @@ MessagePort::MessagePort(Environment* env,
Debug(this, "Created message port");
}

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

@gireeshpunathil gireeshpunathil Feb 23, 2019

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

@addaleax addaleax Feb 23, 2019

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

@gireeshpunathil gireeshpunathil Feb 24, 2019

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.


uv_async_t* MessagePort::async() {
return reinterpret_cast<uv_async_t*>(GetHandle());
}

bool MessagePort::IsDetached() const {
return data_ == nullptr || IsHandleClosing();
}

void MessagePort::TriggerAsync() {
if (IsHandleClosing()) return;
CHECK_EQ(uv_async_send(async()), 0);
CHECK_EQ(uv_async_send(&async_), 0);
}

void MessagePort::Close(v8::Local<v8::Value> close_callback) {
Expand Down Expand Up @@ -643,7 +635,6 @@ void MessagePort::OnClose() {
data_->Disentangle();
}
data_.reset();
delete async();
}

std::unique_ptr<MessagePortData> MessagePort::Detach() {
Expand Down
4 changes: 1 addition & 3 deletions src/node_messaging.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,6 @@ class MessagePort : public HandleWrap {
v8::Maybe<bool> PostMessage(Environment* env,
v8::Local<v8::Value> message,
v8::Local<v8::Value> transfer);
// Deliver a single message into this port's incoming queue.
void AddToIncomingQueue(Message&& message);

// Start processing messages on this port as a receiving end.
void Start();
Expand Down Expand Up @@ -200,9 +198,9 @@ class MessagePort : public HandleWrap {
void OnClose() override;
void OnMessage();
void TriggerAsync();
inline uv_async_t* async();

std::unique_ptr<MessagePortData> data_ = nullptr;
uv_async_t async_;

friend class MessagePortData;
};
Expand Down