worker: make MessagePort uv_async_t inline field#26271
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot commented Feb 23, 2019
@addaleax sadly an error occured when I tried to trigger a build :( |
nodejs-github-bot added the c++ 
src/node_messaging.cc Outdated
There was a problem hiding this comment.
I wanted to ask this earlier as well - what is the consideration behind obtaining this through pointer arithmetic over the other one?
There was a problem hiding this comment.
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 Outdated
There was a problem hiding this comment.
how is this change related?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 left a comment
bnoordhuis left a comment There was a problem hiding this comment.
The removal of MessagePort::AddToIncomingQueue() should arguably be a separate commit.
src/node_messaging.cc Outdated
There was a problem hiding this comment.
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.
addaleaxforce-pushed the messageport-async-inline branch from d42e6e2 to be33b22Compare
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.
addaleaxforce-pushed the messageport-async-inline branch from be33b22 to 7c8f4c2Compare
addaleax commented Mar 1, 2019
Rebased, new CI: https://ci.nodejs.org/job/node-test-pull-request/21081/ |
addaleax added the author ready addaleax commented Mar 1, 2019
PR-URL: #26271 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: #26271 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
6 participants
It’s not obvious why this was a heap allocation in the first place,
but it’s unneccessary. Most other
HandleWraps also store thelibuv 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), orvcbuild test(Windows) passes