-
Notifications
You must be signed in to change notification settings - Fork 500
Refactor: Intorduce new JsValue.ExternalPusher interface to replace StreamSink. #5738
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
15f0180 to
27b2baf
Compare
|
This PR sets up for some other changes I need to make, including:
|
CodSpeed Performance ReportMerging #5738 will degrade performance by 11.15%Comparing Summary
Benchmarks breakdown
Footnotes
|
27b2baf to
e9605b0
Compare
|
Bolted awake at 6AM realizing: I broke e-order. Ugh. |
This is a bit of a hack, but works in certain cases where we know for sure that promises are already resolved, including a case I'm working on in: cloudflare/workerd#5738
c2af264 to
88b293e
Compare
|
The generated output of |
88b293e to
fd2e29a
Compare
|
OK I fixed e-order and actually simplified this quite a bit in the process. It did require a hack in capnp: capnproto/capnproto#2475 But it works OK here. |
fd2e29a to
e7fe4d0
Compare
DO NOT MERGE until the autogate introduced in #5738 is fully rolled out and working. Until then I'm putting this up just to show what we'll be able to remove.
DO NOT MERGE until the autogate introduced in #5738 is fully rolled out and working. Until then I'm putting this up just to show what we'll be able to remove.
| let promises = []; | ||
| promises.push(stub.increment(1)); | ||
| promises.push(stub.increment(1)); | ||
| promises.push(stub.increment(1, abortSignal)); | ||
| promises.push(stub.increment(1)); | ||
| promises.push(stub.increment(1, readableStream)); | ||
| promises.push(stub.increment(1)); | ||
|
|
||
| let results = await Promise.all(promises); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking nit:
| let promises = []; | |
| promises.push(stub.increment(1)); | |
| promises.push(stub.increment(1)); | |
| promises.push(stub.increment(1, abortSignal)); | |
| promises.push(stub.increment(1)); | |
| promises.push(stub.increment(1, readableStream)); | |
| promises.push(stub.increment(1)); | |
| let results = await Promise.all(promises); | |
| const results = await Promise.all([ | |
| stub.increment(1), | |
| stub.increment(1), | |
| stub.increment(1, abortSignal), | |
| stub.increment(1), | |
| stub.increment(1, readableStream), | |
| stub.increment(1), | |
| ]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intentionally didn't write it that way since the order in which the increments are evaluated matters. While JavaScript probably guarantees that elements of an array literal are evaluated in left-to-right order (unlike C++), I didn't really want to write code that assumes anything about expression evaluation order.
e7fe4d0 to
6af735a
Compare
The design of `StreamSink` makes it fairly complicated and somewhat inefficient to implement. For example: * It requires the use of `setPipeline()` so that the caller can start to enable promise pipelining on the `StreamSink` before it actually returns results. * Every call must create a `resultsStreamSink` object in advance, before it knows if there will be any streams in the results. * Generally there's just a lot of contortions involved in supporting it. It occurred to me that a different design is possible: one where we have an object that is created *per-IoContext* (instead of per-call) which can be used to "push" values into that IoContext, so that they can then be referenced as externals by subsequent JsValues. This commit introduces that design, including specifying how it would work to implement `ReadableStream`. Subsequent commits will actually implement this design. Eventually, after everyone in production is updated to understand and then use the new design, we can deprecate and remove StreamSink, thus cleaning up all the mess it created.
capnp PR: capnproto/capnproto#2475 The comment in this file says not to hand-edit it, but I don't understand how to use update-deps.py to update to a not-yet-merged PR...
This doesn't yet support any actual pushed types, this is just the infrastructure needed to make it available. This introduces an autogate which opts into using ExternalPusher for calls. The caller decides (based on the autogate) whether to use ExternalPusher for the whole call. We can turn on the autogate once all call receivers in production understand the new protocol.
6af735a to
e420f35
Compare
|
(rebased to resolve conflicts) |
This was missing, and an earlier version of ExternalPusher broke e-order but I didn't notice initially.
e420f35 to
513626f
Compare
|
Since wd-tests now have |
The design of
StreamSinkmakes it fairly complicated and somewhat inefficient to implement. For example:setPipeline()so that the caller can start to enable promise pipelining on theStreamSinkbefore it actually returns results.resultsStreamSinkobject in advance, before it knows if there will be any streams in the results.It occurred to me that a different design is possible: one where we have an object that is created per-IoContext (instead of per-call) which can be used to "push" values into that IoContext, so that they can then be referenced as externals by subsequent JsValues.
This PR implements this design, guarded behind an autogate. The autogate should not be enabled anywhere in production until all of production has received the code update -- this ensures that all of prod can understand the new protocol before anyone tries to use it.
Once the autogate is fully rolled out, we can delete StreamSink, which should be a pleasing reduction in complexity -- more so than the complexity that this PR adds.
#5744 shows what we'll be able to delete once this is landed. While the total quantity of code is neutral, I feel that StreamSink was a lot more complicated in the implementation details.