WICG / input-for-workers

Specification for exposing input events to workers
Other
25 stars 6 forks source link

How about an alternative api format using streams #5

Closed NavidZ closed 4 years ago

NavidZ commented 5 years ago

As proposed by @yutakahirano in this document we might be able to use the Stream API for this use case. Any reason that this api might be worse or better that what is proposed here?

cc @majido @mustaq @smaug---- @rniwa @majido

smaug---- commented 5 years ago

I don't think we should use Stream API for events. We're after all thinking about an event based API, so rather not invent some totally new event handling mechanism.

yutakahirano commented 5 years ago

@ricea @domenic

NavidZ commented 5 years ago

I agree with @smaug---- here that creating a new mechanism for listening to the events might be a little confusing for developers.

Note that there are some commonalities between this proposal and the other one we have in https://github.com/WICG/input-for-workers/issues/6.

Common attributes:

There are the properties that the original proposal didn't have and I like very much to have them.

However there are a few differences between this and the other one:

@domenic @yutakahirano what do you think?

yutakahirano commented 5 years ago

Thank you, and sorry for the slow response.

I agree that event in general is not very suited to an element in Streams due to some abilities such as stopping propagation, but what's proposed is removing such abilities, and I think a stream of such events is a useful concept.

As far as I understood the data should be consumed in order by the reader in streams. That is not always the case for events. Particularly there are two events (pointermove and pointerrawupdate) that might be sent in different orders that they show up in the input. pointermove is being raf aligned (and hence coalesced with other pointermoves during a frame while the pointerrawupdates will be dispatched to the consumer if they can consume it.

We may be able to construct two independent streams, one for pointermove and one for pointerupdate.

NavidZ commented 5 years ago

I agree that event in general is not very suited to an element in Streams due to some abilities such as stopping propagation, but what's proposed is removing such abilities, and I think a stream of such events is a useful concept. You are correct about stop propagation and whatnot. But what @smaug---- was saying about new way of using the events was that the current codes that handle events in general just add event listeners and will get an event object and start using it. So with the stream API developers should learn a new paradigm for listening to the events.

We may be able to construct two independent streams, one for pointermove and one for pointerupdate.

I'm not sure if I follow this. Let me give you an example. Let's imagine the following set of events:

pointerdown, pointerrawupdate1, pointermove1, pointerrawupdate2, pointermove2, pointerup

and imagine that rAF signal is right after that last pointerup. If the javascript code has handlers (like the normal event handling with listener paradigm that exist today on the web) for all these events (and they don't block the handling thread for long) this is the order of handlers:

running pointerdown handler running pointerrawupdate handler with pointerrawupdate1 running pointerrawupdate handler with pointerrawupdate2 running pointermove handler with pointermove1 coalesced with pointermove2 running pointerup handler

And this order has to be guaranteed.

yutakahirano commented 4 years ago

Ah sorry I misunderstood. Just to make sure, let me rephrase your statement. pointermove events can be reordered and coalesced freely in a frame even there are other events in the frame, right?

NavidZ commented 4 years ago

That statement is correct with respect to pointermove and pointerrawupdate. They can jump over each other and to be more exact pointerrawupdate can jump ahead of pointermove to the extent that the current spec allows.

This might get even more relaxed in the future but not yet. This was the part that I felt stream API makes some guarantees that doesn't quite fit with eventing model aside from the concerns of ergonomic changes to the current event handling mechanisms developers are used to.

yutakahirano commented 4 years ago

Thanks. I guess in our implementation the browser process provides input events, the compositor provides animation frame information. In our current implementation who coalesces mouse events? Is it the renderer process or the browser process?

smaug---- commented 4 years ago

FWIW, in Gecko it is the child process which coalesces mousemoves

NavidZ commented 4 years ago

It is actually in all the stages. In renderer and browser and even in the native platform (depending on the API). Although at this point Chrome only exposes the ones that have been coalesced in the renderer and in the other stages it just drops the old ones and keeps the newest one to send it to the later stages.

yutakahirano commented 4 years ago

I see - I thought coalescing was done mainly/sorely on the browser process and/or the native platform.

We can do:

Web developers can coalesce events by using the coalescing TransofrmStream in any thread.

We could say that we are exposing lower level primitives (events before coalescing and coalescing logic) separately, but we also could say that this is messy.

NavidZ commented 4 years ago

Expose the stream before coalescing is done,

This is not something we should be pursuing. In event dispatching logic, we decided to go down the coalescing and mainly aligning the input with rAF to save the unneeded work that might have been done by the handlers while never getting a chance to render it. So we would like to still keep this behavior by default as it does save quite a bit of CPU processing. So coalescing has to remain in place even for workers. I believe @ojanvafai was also mentioning this from the beginning.

Define a TransformStream which coalesces events,

Can you elaborate on this a little more? How does it handle the out of order events I explained earlier? The main difference between this API and what is proposed in #6 is exactly this queuing notion. That proposal does not make any guarantee about really being a queue or a stream. It just creates a shadow object to get passed to other workers (this part is similar to the stream api) but it can be used similar to the EventTargets on the main thread and workers can add event listeners to it. I agree that it does introduce a new construct and does not use an already defined API like Stream. But stream API has that ordering guarantee that doesn't hold in eventing world.

Transfer the stream without coalescing to the worker thread.

The same reason as I mentioned for the first comment. We should keep the coalescing in place.

yutakahirano commented 4 years ago

We can do:

  • Expose the stream before coalescing is done,
  • Define a TransformStream which coalesces events,
  • Transfer the stream without coalescing to the worker thread.

To be clear, these are one proposal combined, not three options.

My assumption here is, when the user want to receive input events on worker, coalescing is done in the worker thread, not the main thread (ignoring coalescing done in the browser process and/or native platform - they are not affected). Is that correct?

Define a TransformStream which coalesces events,

Can you elaborate on this a little more? How does it handle the out of order events I explained earlier?

A TransformStream is a function between streams. We can define CoalescingStream which pulls events from the source stream and pushes coalesced events to the destination stream.

If a web developer wants to use an event stream on the main thread, they can do something like:

// main thread
// Coalescing is done in the main thread.
const stream = 
  window.getEventStream([‘pointerdown’, ‘pointermove’, ‘pointerup’]).pipeThrough(
    new CoalescingStream());

If a web developer wants to use an event stream on a worker thread, they can do something like:

// main thread
const stream = 
  window.getEventStream([‘pointerdown’, ‘pointermove’, ‘pointerup’]);
worker.postMessage(stream, [stream]);

// worker thread
onmessage = async (evt) => {
  // Coalescing is done in the worker thread.
  const stream = evt.data.pipeThrough(new CoalescingStream());
};

But I have to admit that this is more complex than I originally thought and it may be error-prone from web developer POV.

NavidZ commented 4 years ago

To be clear, these are one proposal combined, not three options.

My bad. I didn't get this at first.

My assumption here is, when the user want to receive input events on worker, coalescing is done in the worker thread, not the main thread (ignoring coalescing done in the browser process and/or native platform - they are not affected). Is that correct?

You are right. Main thread coalescing has nothing to do with this. As a matter of fact events might get more coalesced if main thread is even busy at rAF so main thread might like get one event for all the events happened during the past two frames because if was busy but worker got one for each frame before rAF because if was not busy for whatever reason.

But I have to admit that this is more complex than I originally thought and it may be error-prone from web developer POV.

I see what the TransformStream is from the spec. But I still don't see how it would handle the out of ordering of the events.

Aside from this out of ordering thing, the main concern here that @smaug---- and I share is that it is inventing a new way of handling events for developers which might not be familiar to them. I agree your point about the fact that with this proposal there is no need to invent additional IDLs for us but I think even creating the new ones (for pushing the shadow target like object to the other thread) to keep the same ergonomics of actually handling events (i.e. event listeners and whatnot) for developers when they want to listen is a more consistent experience. @yutakahirano @domenic WDYT?

yutakahirano commented 4 years ago

Sounds reasonable. Please feel free to close this bug.

Thank you for the discussion!