lib: convert transfer sequence to array in js #55317

jazelly · 2024-10-08T09:16:13Z

This PR utilizes the webidl sequence converter to convert transfer in JS layer. Although this is kind of like undoing some efforts made to move the entire strucutredClone to C , it fixes the inconsistency of structuredClone with other runtimes with webidl compliance.

Additionally, this reduces the C to JS overhead, as the iterator function is no longer invoked at C layer, when transfer is an iterator object.

Fixes: #55280
Refs: #50330

nodejs-github-bot · 2024-10-08T09:16:18Z

Review requested:

@nodejs/startup
@nodejs/web-standards

aduh95 · 2024-10-08T11:04:40Z

Tests are failing:

TypeError: webidl.converters.sequence<transfer> is not a function
    at structuredClone (node:internal/structured_clone:35:62)

codecov · 2024-10-08T11:08:08Z

Codecov Report

Attention: Patch coverage is 86.36364% with 9 lines in your changes missing coverage. Please review.

Project coverage is 88.40%. Comparing base (8dbca2d) to head (c9f5904).
Report is 40 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/webidl.js	50.00%	6 Missing ⚠️
src/node_messaging.cc	70.00%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55317       /-   ##
==========================================
- Coverage   88.41%   88.40%   -0.02%     
==========================================
  Files         652      652              
  Lines      186759   186827       68     
  Branches    36097    36048      -49     
==========================================
  Hits       165130   165165       35     
- Misses      14898    14911       13     
- Partials     6731     6751       20

Files with missing lines	Coverage Δ
...internal/bootstrap/web/exposed-window-or-worker.js	`93.70% <100.00%> ( 0.15%)`	⬆️
lib/internal/perf/usertiming.js	`94.14% <100.00%> (ø)`
lib/internal/webstreams/readablestream.js	`98.32% <100.00%> (-0.01%)`	⬇️
lib/internal/worker/js_transferable.js	`98.54% <100.00%> ( 0.54%)`	⬆️
src/node_messaging.cc	`83.56% <70.00%> (-0.50%)`	⬇️
lib/internal/webidl.js	`95.04% <50.00%> (-1.74%)`	⬇️

... and 58 files with indirect coverage changes

jazelly · 2024-10-08T11:10:07Z

Should be fixed now. Thanks for the heads up.

lib/internal/structured_clone.js

jazelly · 2024-10-10T11:00:43Z

cc @joyeecheung This is more like a RFC right now. I'm keen to hear your thoughts on this approach since you initiated the work moving structuredClone to C .

joyeecheung · 2024-10-10T12:24:19Z

The work wasn't really started to move it entirely to C , it was about getting rid of the unnecessary MessageChannel MessagePort abstraction and doing the Message::Serialize/Message::Deserialize directly, which you couldn't do with what was exposed to JS. The most essential part was this

node/src/node_messaging.cc

Lines 1609 to 1613 in b5fb2ff

    
           if (msg->Serialize(env, context, value, transfer_list, Local<Object>()) 
        
                   .IsNothing() || 
        
               !msg->Deserialize(env, context, nullptr).ToLocal(&result)) { 
        
             return; 
        
           }

And this PR is not touching it. The argument conversion should've been done in JS in the first place IMO, just not in the middle of binding, but should be done before the binding is invoked. In fact I actually left a TODO comment in there:

node/src/node_messaging.cc

Lines 1600 to 1601 in b5fb2ff

    
           // TODO(joyeecheung): implement this in JS land to avoid the C   -> JS 
        
           // cost to convert a sequence into an array.

Which I think is exactly what this PR is doing?

lib/internal/structured_clone.js

jazelly · 2024-10-10T20:24:47Z

Yes, exactly what this PR is trying to achieve : )

joyeecheung

LGTM with a nit

lib/internal/worker/js_transferable.js

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`

mcollina

lgtm

nodejs-github-bot · 2024-10-13T13:02:54Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63080/

KhafraDev

I'm not going to block this PR, especially with a green CI, but having an object converter is not correct here. As noted in the PR, a dictionary converter is correct - as an idl dictionary can be represented in js, whilst an idl object cannot.

There are some small issues like hitting object.transfer's getter twice, which is observable via a proxy, and checking the type of options multiple times, but nothing major.

nodejs-github-bot · 2024-10-13T18:11:22Z

Landed in 50b4ada

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matthew Aitken <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matthew Aitken <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matthew Aitken <[email protected]>

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 8, 2024

jazelly force-pushed the js-structured-clone branch 3 times, most recently from 5f3c150 to fa84f31 Compare October 8, 2024 09:25

jazelly force-pushed the js-structured-clone branch from fa84f31 to f64ce5e Compare October 8, 2024 11:08

aduh95 reviewed Oct 8, 2024

View reviewed changes

lib/internal/structured_clone.js Outdated Show resolved Hide resolved

RedYetiDev added web-standards Issues and PRs related to Web APIs and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 10, 2024

joyeecheung reviewed Oct 10, 2024

View reviewed changes

lib/internal/structured_clone.js Outdated Show resolved Hide resolved

lib/internal/structured_clone.js Outdated Show resolved Hide resolved

jazelly force-pushed the js-structured-clone branch from 533ce0a to 476ff6e Compare October 11, 2024 04:15

joyeecheung approved these changes Oct 11, 2024

View reviewed changes

lib/internal/worker/js_transferable.js Outdated Show resolved Hide resolved

lib: convert transfer sequence to array in js

c9f5904

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`

jazelly force-pushed the js-structured-clone branch from 476ff6e to c9f5904 Compare October 11, 2024 21:42

mcollina approved these changes Oct 12, 2024

View reviewed changes

jakecastelli added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 13, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 13, 2024

jasnell approved these changes Oct 13, 2024

View reviewed changes

KhafraDev approved these changes Oct 13, 2024

View reviewed changes

KhafraDev added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 13, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 13, 2024

nodejs-github-bot merged commit 50b4ada into nodejs:main Oct 13, 2024
63 checks passed

aduh95 mentioned this pull request Oct 24, 2024

V23.1.0 proposal #55513

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: convert transfer sequence to array in js #55317

lib: convert transfer sequence to array in js #55317

jazelly commented Oct 8, 2024 •

edited

Loading

nodejs-github-bot commented Oct 8, 2024

aduh95 commented Oct 8, 2024

codecov bot commented Oct 8, 2024 •

edited

Loading

jazelly commented Oct 8, 2024

jazelly commented Oct 10, 2024

joyeecheung commented Oct 10, 2024

jazelly commented Oct 10, 2024

joyeecheung left a comment

mcollina left a comment

nodejs-github-bot commented Oct 13, 2024

KhafraDev left a comment •

edited

Loading

nodejs-github-bot commented Oct 13, 2024

lib: convert transfer sequence to array in js #55317

lib: convert transfer sequence to array in js #55317

Conversation

jazelly commented Oct 8, 2024 • edited Loading

nodejs-github-bot commented Oct 8, 2024

aduh95 commented Oct 8, 2024

codecov bot commented Oct 8, 2024 • edited Loading

Codecov Report

jazelly commented Oct 8, 2024

jazelly commented Oct 10, 2024

joyeecheung commented Oct 10, 2024

jazelly commented Oct 10, 2024

joyeecheung left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Oct 13, 2024

KhafraDev left a comment • edited Loading

Choose a reason for hiding this comment

nodejs-github-bot commented Oct 13, 2024

jazelly commented Oct 8, 2024 •

edited

Loading

codecov bot commented Oct 8, 2024 •

edited

Loading

KhafraDev left a comment •

edited

Loading