How much latitude should implementations have in looking up transformer.transform()? #691
Comments
|
I agree that we shouldn't require implementations to do a double lookup for
`transformer.transform`. Retaining the ability to easily perform stream
transforms without JS involvement is important.
I also agree that your proposed change 2) is better. Yes, it'd be different
from the source and sink handling, but it'd be in a very obvious way. That
is, as soon as you tried changing the transformer's `transform` method (be
it through a straight property set or a prototype change), you'd realize
that it doesn't have an effect.
Additionally, there isn't really a loss of flexibility: you can always e.g.
have the `transform` method forward to another function that's under your
control, or check a flag on the receiver.
|
|
I'm loathe to make the constructor design inconsistent between the three classes. On the other hand, it seems like a low-risk change to cache all the methods on all the constructors, so if this is important, I think we should do that. It's a bit weird from a JS perspective IMO. If we're treating these things as methods, the usual syntax for method calling is We don't want to go back to just treating them as functions, as that breaks cases like https://streams.spec.whatwg.org/#example-both. It would bloat the size of each object a decent bit, e.g. ReadableStreamDefaultController would end up with three more internal slots to store start, pull, and cancel. BTW I think the way you've stated this as a performance cliff between |
|
I'm loathe to make the constructor design inconsistent between the three
classes. On the other hand, it seems like a low-risk change to cache all
the methods on all the constructors, so if this is important, I think we
should do that.
That seems fine to me, too.
It's a bit weird from a JS perspective IMO. If we're treating these things
as methods, the usual syntax for method calling is foo.bar(), not
previouslySavedBar.call(foo). But few people will notice.
It should be clear in the spec, perhaps including in examples, for sure.
Other than that, I think the few people who'd even notice would just apply
one of the trivial workarounds and move on. There really isn't a realistic
way for them to be affected without noticing, I think.
We don't want to go back to just treating them as functions, as that
breaks cases like https://streams.spec.whatwg.org/#example-both.
Fully agreed, calling them with the original object as the receiver makes
sense.
It would bloat the size of each object a decent bit, e.g.
ReadableStreamDefaultController would end up with three more internal
slots to store start, pull, and cancel.
We'd only need to store `pull` and `cancel`, right? `start` is invoked in
the constructor and not needed after that, IINM. I agree that this is
unfortunate, but think it's worth it for the added optimization potential.
|
|
Good point about not needing to store |
|
I feel conflicted about this. An important feature of Streams is being able to plug together standard-shaped pieces and have them work together in a predictable way. If the objects passed to Stream constructors behave differently from Javascript objects in other contexts then we're going against our own design philosophy. Strawman scenario: some future framework uses bytecode for everything on first load to avoid JS compilation overhead. After first load it lazily loads the real Javascript and dynamically swaps all the bytecode functions for the optimizable Javascript versions. Eventually some users of this framework notice that Stream performance is bad and the fact that byte-coding needs to be disabled for sink/source/transformer methods is independently rediscovered several times before finally making it into the documentation. My intuition tells me that optimising for undefined transform() is important, but my intuition about optimisation has been wrong before. |
|
Alternative proposal: scrap the transform() fallback and add an explicit |
I guess I'm not as conflicted because even though they behave differently, it's really not a big deal in practice. I've never really seen JavaScript code that dynamically changes the methods of an object passed elsewhere and expects that to be respected---even though it's probably the case that doing so would work, since most object-accepting code will use Oh, and also, we use the same pattern for custom elements. Here was the thread with arguments for it at that time: WICG/webcomponents#417 |
That's a good discussion of the up- and downsides to this, thanks. I don't know if the same is true for custom elements, though I'd assume so, but as I said above the important part for me is that having this check be required would necessitate entering the JS engine in some fashion, even if it doesn't necessarily entail running JS code itself. (Though recommending developers freeze the object or make the |
I didn't know that custom elements was doing this. That negates my concerns. Thank you. Mark me in favour of "cache method pointers at construction time for ReadableStream, TransformStream and WritableStream". Because this makes no difference in the vast majority of cases, I have no interoperability concerns about changing existing behaviour. |
|
I think we should go directly to #813 (algorithm steps in slots) without doing the method-copying thing as an intermediate step. |
|
Oh, wow, I didn't realize they solved the same problem. Good point. I'll set aside my half-done branch that does method copying and switch to working on algorithms in slots. |
|
So, with #857 merged, where does this leave us? Do we still want to pull off the properties at creation time? It seems with the current spec text we still have the problem where |
|
Yes, I plan to do this soon, probably tomorrow. Aside from providing optimisation opportunities I expect it will be a good simplification for TransformStream. |
Stream classes are changing to get methods from the underlying object during construction rather than when the methods are called. Change expectations of when method lookup happens. See whatwg/streams#691 for background.
Lookup methods on underlyingSource, underlyingSink and transformer in the constructor. The main benefit is that identity transforms can be simply implemented without touching Javascript. Other less critical optimisation opportunities are unlocked for other stream types. The algorithm construction for used-supplied methods is changed so that a different algorithm is constructed depending on whether or not the method was supplied. Two new abstract operations are added: * GetMethod works like GetV but throws if the value is neither undefined nor a function. * PromiseInvoke works like PromiseInvokeOrNoop but doesn't check for undefined. PromiseInvokeOrNoop is no longer used and has been removed. Constructors now throw for invalid method parameters, which results in some test expectation changes. Changing the methods or prototype on the underlying object after construction no longer does anything. Other than these edge cases there are no web-developer visible changes. Fixes whatwg#691.
|
WIP at #860. The test changes web-platform-tests/wpt#8519 are unexpectedly nice. There are less edge cases. Even though the new semantics are more unusual than the old ones, subjectively they seem less weird. Sadly no TransformStream tests failed due to the change. I have a separate work in progress to fix that :-) |
Stream classes are changing to get methods from the underlying object during construction rather than when the methods are called. Change expectations of when method lookup happens. Includes new tests to verify that TransformStream and TransformStreamDefaultController have the correct properties, and that those properties have the correct properties, and that transformer objects are called in the correct ways, and not called in unexpected ways. See whatwg/streams#691 for background.
Lookup methods on underlyingSource, underlyingSink and transformer in the constructor. The main benefit is that identity transforms can be simply implemented without touching Javascript. Other less critical optimisation opportunities are unlocked for other stream types. The algorithm construction for used-supplied methods is changed so that a different algorithm is constructed depending on whether or not the method was supplied. PromiseCall() replaces PromiseInvokeOrNoop(); it is similar but accepts a function argument rather than a string, and doesn't permit it to be undefined. CreateAlgorithmFromUnderlyingMethod() is a new operation that encapsulates the logic to create an algorithm that calls the method or does nothing depending on whether or not the method is defined or not. It is used for all user-supplied methods except transformer.transform(), which has custom fallback behaviour. In many cases the controller argument must be included in the arguments to CreateAlgorithmFromUnderlyingMethod(). In order to permit algorithms to be created before the controller is fully initialised, the creation of controller objects has moved into the caller of the SetUpFooStreamBarController(). Constructors now throw for invalid method parameters, which results in some test expectation changes. Changing the methods or prototype on the underlying object after construction no longer does anything. Other than these edge cases there are no web-developer visible changes. Some SetUpFooController operations were erroneously marked as nothrow in the standard text and have been fixed, along with callers correctly annotated with "?". Fixes #691.
Pull request #689 verifies that transformer.transform is looked up in exactly the same way as the reference implementation. But this puts pretty severe restrictions on possible optimisations. For example,
new TransformStream()can be optimised to skip Javascript altogether, butnew TransformStream({})cannot. Here are some use cases for a TransformStream with no transform() method:I see two ways we could permit optimisation here:
I think for developers 2. is better than 1. because it will at least be consistent between implementations even if it's different from the way sources and sinks work.
The semantics we provide for sources and sinks are that we treat them exactly like an object, including expected Javascript behaviours like dynamically changing prototypes.
My feeling is that complying with this expectation is important, even though only a tiny proportion of developers may take advantage of the flexibility. On the other hand, falling off a performance cliff because you wrote
new TransformStream({})instead ofnew TransformStream()is suboptimal. Making identity transforms always be slower just to avoid surprising performance behaviour would also be bad.Thoughts?
The text was updated successfully, but these errors were encountered: