Skip to content

stream: set stream prototype to closest transferable superclass - #55067

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
jazelly:fix-54603
Sep 24, 2024
Merged

stream: set stream prototype to closest transferable superclass#55067
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
jazelly:fix-54603

Conversation

@jazelly

@jazellyjazelly commented Sep 22, 2024

Copy link
Copy Markdown
Member

The latest WPT test requires this behaviour. The ReflectConstruct way did this well, but there was an attempt to reduce the ReflectConstruct overhead breaking this, since it introduced a "closer" superclass under Readable (or Writable etc.).

Fixes: #54603
Refs: #50107

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Sep 22, 2024
@codecov

codecovBot commented Sep 22, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.23%. Comparing base (cfe58cf) to head (c12600d).
Report is 422 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #55067 +/- ##
=======================================
Coverage 88.23% 88.23% =======================================
Files 652 652 Lines 183912 183926 +14 Branches 35865 35858 -7 =======================================
+ Hits 162272 162290 +18 - Misses 14915 14917 +2 + Partials 6725 6719 -6 
Files with missing linesCoverage Δ
lib/internal/webstreams/readablestream.js98.32% <100.00%> (+<0.01%)⬆️
lib/internal/webstreams/transformstream.js99.56% <100.00%> (+<0.01%)⬆️
lib/internal/webstreams/writablestream.js99.85% <100.00%> (+<0.01%)⬆️

... and 31 files with indirect coverage changes

Comment threadtest/parallel/test-structuredClone-global.js Outdated
@avivkelleravivkeller added the web-standards Issues and PRs related to Web APIs label Sep 22, 2024
Comment threadlib/internal/webstreams/readablestream.js Outdated
@avivkelleravivkeller added the needs-benchmark-ci PR that need a benchmark CI run. label Sep 22, 2024
Comment threadtest/parallel/test-structuredClone-global.js Outdated
@jazelly
jazellyforce-pushed the fix-54603 branch 2 times, most recently from 6abe0ca to feaf4d0CompareSeptember 23, 2024 01:13
@avivkelleravivkeller 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 Sep 23, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@jakecastelli

Copy link
Copy Markdown
Member

@avivkeller

Copy link
Copy Markdown
Member

Benchmark LGTM

 confidence improvement accuracy (*) (**) (***)
webstreams/creation.js kind='ReadableStream.tee' n=50000 *** -2.66 % ±0.47% ±0.63% ±0.82%
webstreams/creation.js kind='ReadableStream' n=50000 0.70 % ±0.70% ±0.93% ±1.21%
webstreams/creation.js kind='ReadableStreamBYOBReader' n=50000 -0.49 % ±4.38% ±5.83% ±7.60%
webstreams/creation.js kind='ReadableStreamDefaultReader' n=50000 0.53 % ±1.89% ±2.52% ±3.27%
webstreams/creation.js kind='TransformStream' n=50000 *** -1.67 % ±0.78% ±1.05% ±1.38%
webstreams/creation.js kind='WritableStream' n=50000 0.59 % ±0.65% ±0.87% ±1.13%
webstreams/js_transfer.js n=10000 payload='ReadableStream' *** 0.99 % ±0.46% ±0.61% ±0.80%
webstreams/js_transfer.js n=10000 payload='TransformStream' *** -1.61 % ±0.26% ±0.35% ±0.46%
webstreams/js_transfer.js n=10000 payload='WritableStream' ** -0.69 % ±0.51% ±0.68% ±0.89%
webstreams/pipe-to.js highWaterMarkW=1024 highWaterMarkR=1024 n=500000 -0.43 % ±1.04% ±1.39% ±1.81%
webstreams/pipe-to.js highWaterMarkW=1024 highWaterMarkR=2048 n=500000 -0.18 % ±1.04% ±1.39% ±1.81%
webstreams/pipe-to.js highWaterMarkW=1024 highWaterMarkR=4096 n=500000 0.48 % ±1.08% ±1.43% ±1.86%
webstreams/pipe-to.js highWaterMarkW=1024 highWaterMarkR=512 n=500000 0.47 % ±1.14% ±1.51% ±1.97%
webstreams/pipe-to.js highWaterMarkW=2048 highWaterMarkR=1024 n=500000 -0.17 % ±1.15% ±1.53% ±2.00%
webstreams/pipe-to.js highWaterMarkW=2048 highWaterMarkR=2048 n=500000 0.78 % ±0.91% ±1.21% ±1.57%
webstreams/pipe-to.js highWaterMarkW=2048 highWaterMarkR=4096 n=500000 0.60 % ±1.28% ±1.70% ±2.22%
webstreams/pipe-to.js highWaterMarkW=2048 highWaterMarkR=512 n=500000 -0.03 % ±1.11% ±1.48% ±1.93%
webstreams/pipe-to.js highWaterMarkW=4096 highWaterMarkR=1024 n=500000 -1.10 % ±1.13% ±1.50% ±1.95%
webstreams/pipe-to.js highWaterMarkW=4096 highWaterMarkR=2048 n=500000 -0.89 % ±1.03% ±1.36% ±1.78%
webstreams/pipe-to.js highWaterMarkW=4096 highWaterMarkR=4096 n=500000 0.56 % ±1.06% ±1.41% ±1.83%
webstreams/pipe-to.js highWaterMarkW=4096 highWaterMarkR=512 n=500000 0.08 % ±1.12% ±1.49% ±1.94%
webstreams/pipe-to.js highWaterMarkW=512 highWaterMarkR=1024 n=500000 0.08 % ±1.05% ±1.40% ±1.83%
webstreams/pipe-to.js highWaterMarkW=512 highWaterMarkR=2048 n=500000 0.11 % ±1.03% ±1.37% ±1.78%
webstreams/pipe-to.js highWaterMarkW=512 highWaterMarkR=4096 n=500000 -0.58 % ±1.01% ±1.35% ±1.75%
webstreams/pipe-to.js highWaterMarkW=512 highWaterMarkR=512 n=500000 -0.25 % ±1.03% ±1.37% ±1.79%
webstreams/readable-async-iterator.js n=100000 -0.05 % ±1.27% ±1.69% ±2.21%
webstreams/readable-read.js type='byob' n=100000 -0.39 % ±0.91% ±1.21% ±1.58%
webstreams/readable-read.js type='normal' n=100000 0.85 % ±1.26% ±1.68% ±2.18%
Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 28 comparisons, you can thus
expect the following amount of false-positive results:
1.40 false positives, when considering a 5% risk acceptance (*, **, ***),
0.28 false positives, when considering a 1% risk acceptance (**, ***),
0.03 false positives, when considering a 0.1% risk acceptance (***)

@aduh95aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 24, 2024
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 24, 2024
@nodejs-github-bot
nodejs-github-bot merged commit 0e52836 into nodejs:mainSep 24, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 0e52836

@avivkeller

Copy link
Copy Markdown
Member

🚀 Great! I'll rebase this into the WPT update PR, and it should be ready to review then :-)

@jazelly
jazelly deleted the fix-54603 branch September 25, 2024 03:40
targos pushed a commit that referenced this pull request Oct 4, 2024
PR-URL: #55067Fixes: #54603
Refs: #50107
Reviewed-By: Mattias Buelens <mattias@buelens.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@aduh95aduh95 mentioned this pull request Oct 9, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.needs-benchmark-ciPR that need a benchmark CI run.needs-ciPRs that need a full CI run.web streamsweb-standardsIssues and PRs related to Web APIs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Node.js structuredClone objects do not preserve prototypes

8 participants

@jazelly@nodejs-github-bot@jakecastelli@avivkeller@MattiasBuelens@targos@aduh95@marco-ippolito