Remove JS stream usage #29881
Draft
Remove JS stream usage #29881
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@bors-servo try=wpt |
bors-servo
added a commit
that referenced
this pull request
Jun 16, 2023
Remove JS stream usage Rewrite readable stream code to avoid use of JS streams. Fixes #29088.
|
💔 Test failed - checks-github |
|
@bors-servo try=wpt |
bors-servo
added a commit
that referenced
this pull request
Jun 16, 2023
Remove JS stream usage Rewrite readable stream code to avoid use of JS streams. Fixes #29088.
|
💔 Test failed - checks-github |
|
@bors-servo try=wpt |
bors-servo
added a commit
that referenced
this pull request
Jun 16, 2023
Remove JS stream usage Rewrite readable stream code to avoid use of JS streams. Fixes #29088.
|
💔 Test failed - checks-github |
|
@bors-servo try=wpt |
bors-servo
added a commit
that referenced
this pull request
Jun 16, 2023
Remove JS stream usage Rewrite readable stream code to avoid use of JS streams. Fixes #29088.
|
Test results for linux-wpt-layout-2013 from try job (#5286601539): Flaky unexpected result (19)
Stable unexpected results that are known to be intermittent (11)
Stable unexpected results (84)
|
|
💔 Test failed - checks-github |
|
Lots of those failures are expected since the current DOM ReadableStream interface is not standards-compliant at all. It's worth investigating the timeouts, though. |
|
Some timeouts (like |
|
@bors-servo try=wpt |
bors-servo
added a commit
that referenced
this pull request
Jun 17, 2023
Remove JS stream usage Rewrite readable stream code to avoid use of JS streams. Fixes #29088.
|
Test results for linux-wpt-layout-2013 from try job (#5296240437): Flaky unexpected result (8)
Stable unexpected results that are known to be intermittent (12)
Stable unexpected results (55)
|
|
💔 Test failed - checks-github |
|
The |
|
@bors-servo try=wpt |
bors-servo
added a commit
that referenced
this pull request
Jun 19, 2023
Remove JS stream usage Rewrite readable stream code to avoid use of JS streams. Fixes #29088.
|
Test results for linux-wpt-layout-2013 from try job (#5315296557): Flaky unexpected result (22)
Stable unexpected results that are known to be intermittent (16)
Stable unexpected results (83)
|
|
💔 Test failed - checks-github |
|
☔ The latest upstream changes (presumably #29934) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Sorry for causing the merge conflict on this change. I'm happy to rebase it and bring it back to life. |
|
Don't worry, I'll take care of it. |
|
@jdm Should this one be closed for now since we have updated SpiderMonkey by reverting the streams usage? |
|
Nope! I still think we want to proceed with this work. |
|
It seems like it'd be better if, now that there's a temporary solution for upgrading SM, this PR could be reworked to add non-SM streams as a compile-time feature, so it could be developed over time, perhaps by multiple contributors, without regressions. |
|
That's a fair point! My instinct is that doing all that work in one PR is going to be pretty overwhelming, though. I'm open to ideas if how to split up the work in useful ways! |
gterzian
reviewed
Dec 7, 2023
| struct ExternalUnderlyingSourceController { | ||
| /// Loosely matches the underlying queue, | ||
| /// <https://streams.spec.whatwg.org/#internal-queues> | ||
| buffer: RefCell<Vec<u8>>, | ||
| #[ignore_malloc_size_of = "Rc is hard"] | ||
| pending_reader: RefCell<Option<Rc<Promise>>>, |
There was a problem hiding this comment.
I would rather keep those on the dom struct, in some ways ExternalUnderlyingSourceController doesn't really belong here at all because it's the "native" pure Rust part, and should eventually perhaps be separated out into shared/script(similar to Blob and Messageport).
| assert!(self.pending_reader.borrow().is_none()); | ||
| *self.pending_reader.borrow_mut() = Some(promise.clone()); | ||
| let buflen = self.buffer.borrow().len(); | ||
| self.maybe_signal_available_bytes(cx, &promise.global(), buflen); |
There was a problem hiding this comment.
The problem with this approach is that it will only wake up the reader in reponse to enqueue_native, and in practice the queuing could also be done by a JS underlying source(example).
That was the part that SM dealt with internally, I think.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rewrite readable stream code to avoid use of JS streams. Fixes #29088.