src: implement structuredClone in native#50330
src: implement structuredClone in native#50330nodejs-github-bot merged 4 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
|
will this fix #49940 too? |
mcollina
left a comment
There was a problem hiding this comment.
lgtm
Am I correct in assuming this will improve all messaging through worker threads?
|
@mcollina no, if you look at the changes this instead of creating a MessageChannel object in JS land creates it in C++ land - basically moving that part from JS to C++, see https://github.com/nodejs/node/pull/50330/files#diff-512ab8348327408c9a5ba3f7c60d8f8977a672a22b96345b8cfc2562936062f5R1582-R1583 |
d04776a to
81ff9d9
Compare
|
FYI this is not yet ready because I want to see if we can just avoid the message ports all together without doing a lot of refactoring (there are a lot of code in place to check that the port isn't in the arguments, which then rely on having a port instance to check on) - if not, I'll just try getting this in first before following up with a PR to get rid of the message ports |
81ff9d9 to
d7538ee
Compare
|
From what I can tell simply using the existing |
I don't think so, the assertion comes from the ReadIterable implementation and that's still unfixed - on the other hand it might be better to just implement that method in JS land and do a glue of structuredClone with that method. This calls into JS so many times that doing it in C++ would be slower. I can do that as a follow-up. |
|
I removed the messaging namespace rename (this should still be done in another PR though) and the BindingData because it now no longer relies on MessagePorts. The benchmark numbers are improved a bit too due to the removal of unnecessary hoops: This should be ready for review now - I'd appreciate another pair of eyeballs to confirm that this still yields equivalent results. |
d7538ee to
7d8ecdd
Compare
Simplify the implementation by implementing it directly in C++. This improves performance and also makes structuredClone supported in custom snapshots.
7d8ecdd to
b69261c
Compare
|
Landed in c3a41d8 |
Simplify the implementation by implementing it directly in C++. This improves performance and also makes structuredClone supported in custom snapshots. PR-URL: nodejs#50330 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Simplify the implementation by implementing it directly in C++. This improves performance and also makes structuredClone supported in custom snapshots. PR-URL: #50330 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Simplify the implementation by implementing it directly in C++. This improves performance and also makes structuredClone supported in custom snapshots. PR-URL: #50330 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
This commit lets `tranfer` passed to `structuredClone` get validated at JS layer by doing webidl conversion. This avoids the C++ to JS function call overhead in the native implementaiton of `structuredClone` PR-URL: #55317 Fixes: #55280 Refs: #50330 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
This commit lets `tranfer` passed to `structuredClone` get validated at JS layer by doing webidl conversion. This avoids the C++ to JS function call overhead in the native implementaiton of `structuredClone` PR-URL: nodejs#55317 Fixes: nodejs#55280 Refs: nodejs#50330 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
This commit lets `tranfer` passed to `structuredClone` get validated at JS layer by doing webidl conversion. This avoids the C++ to JS function call overhead in the native implementaiton of `structuredClone` PR-URL: #55317 Fixes: #55280 Refs: #50330 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
This commit lets `tranfer` passed to `structuredClone` get validated at JS layer by doing webidl conversion. This avoids the C++ to JS function call overhead in the native implementaiton of `structuredClone` PR-URL: nodejs#55317 Fixes: nodejs#55280 Refs: nodejs#50330 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
This commit lets `tranfer` passed to `structuredClone` get validated at JS layer by doing webidl conversion. This avoids the C++ to JS function call overhead in the native implementaiton of `structuredClone` PR-URL: nodejs#55317 Fixes: nodejs#55280 Refs: nodejs#50330 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
This commit lets `tranfer` passed to `structuredClone` get validated at JS layer by doing webidl conversion. This avoids the C++ to JS function call overhead in the native implementaiton of `structuredClone` PR-URL: nodejs#55317 Fixes: nodejs#55280 Refs: nodejs#50330 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
This commit lets `tranfer` passed to `structuredClone` get validated at JS layer by doing webidl conversion. This avoids the C++ to JS function call overhead in the native implementaiton of `structuredClone` PR-URL: #55317 Fixes: #55280 Refs: #50330 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
This commit lets `tranfer` passed to `structuredClone` get validated at JS layer by doing webidl conversion. This avoids the C++ to JS function call overhead in the native implementaiton of `structuredClone` PR-URL: #55317 Fixes: #55280 Refs: #50330 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Simplify the implementation by implementing it directly in C++. This improves performance and also makes structuredClone supported in custom snapshots.
Local benchmark numbers: