Start implementing stream internal slots and algos by gterzian · Pull Request #32730 · servo/servo

gterzian · 2024-07-08T15:10:02Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #___ (GitHub issue number if applicable)

There are tests for these changes OR
These changes do not require tests because ___

components/script/dom/readablestream.rs

components/script/dom/readablestreamdefaultcontroller.rs

wusyong

LGTM now, but I still don't have the permissions. Could I get some access to review and add labels?

servo-wpt-sync · 2024-07-11T10:09:00Z

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

Signed-off-by: gterzian <[email protected]>

gterzian · 2024-07-12T04:27:30Z

Ok, I also turned-off the old streams, fixed some things, and as a kind of milestone, https://github.com/servo/servo/blob/496ce717c5343984e0f3da00d223604eff03893c/tests/wpt/tests/fetch/api/basic/stream-response.any.js is now passing again(which covers part of the "native" streaming I handled in this PR).

So I'd say let review and merge this one, so that work can resume in parallel on:

The BYOB controller/reader part(note we were not running tests for that part in the previous implementation, even though SM seems the implement it)
Various TODOs in the default controller/reader.
other stuff?

jdm · 2024-07-12T13:37:59Z

I'll try to take a look at this tonight!

sagudev · 2024-07-12T15:50:36Z

note we were not running tests for that part in the previous implementation, even though SM seems the implement it

Doesn't that mean that readable-streams test's are run (not skipped)?

components/script/dom/readablestream.rs

jdm · 2024-07-13T02:13:24Z

components/script/dom/readablestreamdefaultcontroller.rs

+
+    pub fn enqueue_chunk(&self, mut chunk: Vec<u8>) {
+        let mut buffer = self.buffer.borrow_mut();
+        chunk.append(&mut buffer);


Doesn't this push the existing buffer onto the end of the newest chunk?

Yes that seems to be the case.

On this and the comment below: I looked into this and actually we should re-structure this queue concept to allow to any JS value being a chunk, a first step of which I have done in the last commit. There is still a "native" chunk variant, which I guess we could optimize.

jdm · 2024-07-13T02:17:25Z

components/script/dom/readablestreamdefaultreader.rs

+    /// <https://streams.spec.whatwg.org/#readablestreamgenericreader-stream>
+    stream: Dom<ReadableStream>,
+
+    #[ignore_malloc_size_of = "Rc is hard"]


nit: There are no Rc values involved in this field.

The read request contains a Rc<Promise>.

components/script/dom/readablestream.rs

jdm · 2024-07-14T22:12:13Z

components/script/dom/underlyingsourcecontainer.rs

        let cx = GlobalScope::get_cx();
        rooted!(in(*cx) let mut underlying_source_dict = UndefinedValue());
        if let UnderlyingSourceType::Js(ref js_source) = underlying_source_type {
+            #[allow(unsafe_code)]


nit: it makes more sense to have this annotation at the method level.

jdm · 2024-07-14T22:14:51Z

components/script/dom/readablestreamdefaultreader.rs

 impl ReadRequest {
    /// <https://streams.spec.whatwg.org/#read-request-chunk-steps>
+    #[allow(unsafe_code)]
+    #[allow(crown::unrooted_must_root)]


Why is this annotation required?

I was able to remove it by avoiding to assign the read result to a temporary variable.

components/script/dom/readablestreamdefaultreader.rs

+                rooted!(in(*cx) let mut rval = UndefinedValue());
+                let result = RootedTraceableBox::new(Heap::default());
+                result.set(*rval);
+                let result = ReadableStreamReadResult {


gterzian · 2024-07-15T08:11:17Z

Doesn't that mean that readable-streams test's are run (not skipped)?

@sagudev

yes but the byte stream stuff is in a separate folder, at https://github.com/servo/servo/tree/main/tests/wpt/tests/streams/readable-byte-streams, so we were never running them.

jdm · 2024-07-15T11:54:06Z

components/script/dom/readablestreamdefaultcontroller.rs

+#[allow(crown::unrooted_must_root)]
 struct PullAlgorithmFulfillmentHandler {
-    controller: DomRoot<ReadableStreamDefaultController>,
+    controller: Dom<ReadableStreamDefaultController>,


I don't think this is safe, since these structs aren't visible to the GC. That's why I recommend Trusted here.

That's why I recommend Trusted here.

I just tried it and it breaks Callback: JSTraceable + MallocSizeOf .

these structs aren't visible to the GC

But they end-up on the PromiseNativeHandler, which is a dom_struct.

I've strengthened this by keeping the controller rooted while the native promise handler is setup.

jdm · 2024-07-15T13:42:39Z

b65ba8d is how I would support dictionary callback values.

gterzian · 2024-07-15T15:29:29Z

@jdm thank you!

@wusyong @Taym95 I will address the remaining points from the review, and then I think we need to merge and work in parallel, because I am starting to run into some complicated "how to do this in JS" stuff that warrant PRs in and of themselves...

wusyong · 2024-07-16T03:55:17Z

@wusyong @Taym95 I will address the remaining points from the review, and then I think we need to merge and work in parallel, because I am starting to run into some complicated "how to do this in JS" stuff that warrant PRs in and of themselves...

I thought this PR would be merged earlier so I was waiting for it. Byte controller and reader is pretty similar to default one. So I believe once we have done the default ones, the byte part will be faster. I'll review the PR again pretty soon.

wusyong

@jdm @gterzian Could we merge this today? I believe @gterzian mentioned we could do more aggressive mergine since this is targeted to the feature branch.
There are some trivial improvements like EnqueuedValue and maybe QueueWithSize should share with byob controller. But I think it could wait for there's PR for such controller implementation.

Edit: I also hope we can update the feature branch to catch up main branch since there are some more commit over weeks.

gterzian · 2024-07-19T09:04:02Z

Ok let's do it! @jdm I have addressed all of your comments, although I still had a few questions(about the rooting), but this had been noted in a TODO as needed to revisit...

gterzian marked this pull request as draft July 8, 2024 15:10

gterzian force-pushed the rs-internal-slots-starts branch 7 times, most recently from a5afb8f to 2fa3292 Compare July 9, 2024 12:32

wusyong requested changes Jul 10, 2024

View reviewed changes

components/script/dom/readablestream.rs Show resolved Hide resolved

components/script/dom/readablestreamdefaultcontroller.rs Show resolved Hide resolved

gterzian force-pushed the rs-internal-slots-starts branch 3 times, most recently from 6906fa1 to c50f7f3 Compare July 10, 2024 12:29

wusyong approved these changes Jul 10, 2024

View reviewed changes

gterzian force-pushed the rs-internal-slots-starts branch 10 times, most recently from 585110e to 1daab8d Compare July 11, 2024 10:08

gterzian force-pushed the rs-internal-slots-starts branch from 1daab8d to 9d7f4aa Compare July 11, 2024 10:17

gterzian added 5 commits July 11, 2024 18:17

implement basic internal slots, with todos

0ce3133

Signed-off-by: gterzian <[email protected]>

enum for controller

f2e23dd

Signed-off-by: gterzian <[email protected]>

re-implement native controller methods

0a298b7

Signed-off-by: gterzian <[email protected]>

add calling into pull algo

0b6f761

Signed-off-by: gterzian <[email protected]>

more details on chunk enqueuing

7ee16c2

Signed-off-by: gterzian <[email protected]>

gterzian marked this pull request as ready for review July 12, 2024 04:24

gterzian added 2 commits July 12, 2024 12:28

tidy

997259f

clean-up js conversions in read req handlers

2d2513a

jdm requested changes Jul 14, 2024

View reviewed changes

gterzian added 2 commits July 15, 2024 18:49

add queue with sizes concept

24fcefc

use dom in pull promise handlers

09f1360

jdm reviewed Jul 15, 2024

View reviewed changes

Demonstrate using dictionary as callback this object.

b65ba8d

move value with size to a struct

d02783b

gterzian force-pushed the rs-internal-slots-starts branch 2 times, most recently from 78e562b to d02783b Compare July 15, 2024 15:25

gterzian added 6 commits July 15, 2024 23:33

fmt

17d5862

put readable stream state in a cell

a58fcb8

nits in expectations

b1488c0

remove allow unroot by passing read result directly to promise resolving

53a1051

tidy

0befa08

root default controller inside call_pull_if_needed

79b7f15

wusyong self-requested a review July 16, 2024 03:55

wusyong approved these changes Jul 19, 2024

View reviewed changes

gterzian merged commit 4459d7e into servo:readablestream-re-implementation Jul 19, 2024

wusyong mentioned this pull request Jul 22, 2024

Upcoming Servo issue triage meeting on July 22 2024 servo/project#93

Closed

gterzian mentioned this pull request Jul 31, 2024

ReadableStream: finish default controller and reader impl #32898

Closed

29 tasks

Uh oh!

Conversation

gterzian commented Jul 8, 2024

Uh oh!

Uh oh!

Uh oh!

wusyong left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

servo-wpt-sync commented Jul 11, 2024

Uh oh!

gterzian commented Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdm commented Jul 12, 2024

Uh oh!

sagudev commented Jul 12, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

gterzian commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gterzian Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdm commented Jul 15, 2024

Uh oh!

gterzian commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wusyong commented Jul 16, 2024

Uh oh!

wusyong left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gterzian commented Jul 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wusyong left a comment •

edited

Loading

gterzian commented Jul 12, 2024 •

edited

Loading

gterzian commented Jul 15, 2024 •

edited

Loading

gterzian Jul 15, 2024 •

edited

Loading

gterzian commented Jul 15, 2024 •

edited

Loading

wusyong left a comment •

edited

Loading