RFC Remove event pooling

[syranide]

I couldn't really find anyone on IRC who really knew why we do this. It's annoying to debug and people often don't understand what's happening.

I understand the idea of decreasing the pressure on the GC, but I fail to see how pooling events is going to matter, we're talking worst-case of a consistent stream of about 2 events per frame (give or take, or?). If all we do is dispatch events and there is no render, any pressure on the GC should be imperceptible and any half-decent GC shouldn't find it hard to deal short-lived objects. If any of those update an immutable store we're going to see a handful of objects created and unreferenced, if any of those cause a React render, we might be seeing hundreds of objects created and unreferenced.

For internal classes, pooling is great no matter what really, but we're talking about a public API that's also rather common and standardized. This is not intuitive behavior and so event pooling comes at a great cost, but from where I'm standing there doesn't seem to be an equal or greater benefit.

PS. Only after doing this PR did I discover that there actually is a event.persist() method, if we stick to pooled events, we really should document it as it definitely makes it less of a pain.

PS2. PR because I can, not because I care :)

[syranide]

cc @jordwalke ?

[zpao]

I would counter-argue and say anything we can do to reduce GC is a win. While I agree that this isn't the worst place for allocations, I don't see any really compelling reason to take a step backwards. There's a tradeoff, but I wouldn't call it a "great cost", I can count on 1 hand the number of times it's come up in the past year. Logging events is annoying but as you found out easily solved so I think we should leave this as is (and document better).

@yungsters @petehunt @sebmarkbage

[syranide]

I understand your point, but from my perspective I've seen a handful of people get confused by it on IRC (which indicates that it's at least unintuitive behavior):

1. If reducing GC is an overriding goal, then does it really make sense to promote immutability which creates long-lived objects which are harder on the GC? Transient non-cyclic event objects should be near trivial for a GC. Shouldn't pooled descriptors be of utmost importance?
2. "We want to keep the API as simple as possible and help developers fall into the pit of success. Enabling bad patterns with bad APIs is not success." is a clearly stated goal of React it seems. On the surface, pooled events contradict that (imho, it's not necessarily wrong though).
3. There is a huge non-obvious issue today. Consider someone relaying events (like a custom button) and do event.targetComponent = this to relay additional information, perhaps not good practice, but it's not far-fetched. Significant and indefinite memory leaks ensue, this is fixable, but adds more overhead.

My intuition (which may be completely off) is that the overhead of dealing with pooled events is greater than the cost incurred on the GC (which would probably occur during an idle frame anyway). Even if that isn't that case, I just can't imagine pooling events to actually have a measurable impact in non-synthetic tests. If your information suggests otherwise then by all means, shoot me down, but it feels like premature optimization to me.

[sebmarkbage]

IIRC we added or fixed event pooling after seeing a lot of GC thrash for mouse move events which have very high frequency (we originally didn't support these). Even when nothing is acting on these.

Pooled descriptors is indeed something we want to introduce to avoid thrash in extreme cases where updates happen on every mouse or
touch move. We need to be able to operate well in edge cases too.

They're generally easy to predict when they can be reused because you're not expected to hold on to anything created in the render function of a component. Expect possibly for animation which would need to tie into the pooling system.

DOM events are also reused in a similar way. I'm not saying it's right. Ideally we should encourage more immutable behavior but there is existing precedence for this behavior.

Mutating React objects is already considered bad practice but we could do a Object.preventExtension call to explicitly ensure that nobody adds additional properties to the object.

On May 28, 2014, at 2:44 AM, Andreas Svensson notifications@github.com wrote:

I understand your point, but from my perspective I've seen a handful of people get confused by it on IRC (which indicates that it's at least unintuitive behavior):

If reducing GC is an overriding goal, then does it really make sense to promote immutability which creates long-lived objects which are harder on the GC? Transient non-cyclic event objects should be near trivial for a GC. Shouldn't pooled descriptors be of outmost importance otherwise?

"We want to keep the API as simple as possible and help developers fall into the pit of success. Enabling bad patterns with bad APIs is not success." is a clearly stated goal of React it seems. On the surface, pooled events contradict that (imho, it's not necessarily wrong though).

There is a huge non-obvious issue today. Consider someone relaying events (like a custom button) and do event.targetComponent = this to relay additional information, perhaps not good practice, but it's not far-fetched. Significant and indefinite memory leaks ensue, this is fixable, but adds more overhead.

My intuition (which may be completely off) is that the overhead of dealing with pooled events, would take away the minimal cost it would incur the GC (which would probably occur during an idle frame anyway). Even if that isn't that case, I just can't imagine pooling events to actually have a measurable impact in non-synthetic tests. If your information suggests otherwise then by all means, shoot me down, but it fees like "premature optimization" to me.

—
Reply to this email directly or view it on GitHub.

[jordwalke]

Yes, pooling was in response to an actual issue. And yes, it's not the worst offender. But we should also be fixing the other offenders. For example, did you know we're cloning every props object every time a component reconciles!

[syranide]

@sebmarkbage For mouse move, couldn't we simply make the instantiation of the event object lazy? That would take care of the case where no one is listening. As for descriptors, isn't that contradictory to the recent commits? I may have misinterpreted, but I remember hearing that being able to reuse descriptors since that commit to be a feature (or at least, that some people chose to interpret it that way).

@jordwalke That's where it doesn't really add up in my head, if we have significantly worse offenders, have we been able to observe improvements from pooling just events? I'm at loss as to how 1-3 transient objects per frame can cause thrashing by itself (the browser doesn't pool event objects itself). Are you sure there weren't external factors at fault? Like say, indefinitely appending to a string/array for every mouse move, which could mess up memory allocation.

(PS. I think this is one of those things that more makes sense when/if more features in React adopt it)

[sebmarkbage]

@syranide Descriptors can be reused in the same owner subtree which created it. E.g.

render() {
  var a = <div />;
  return <div>{a}{a}</div>;
}

If this render method is reexecuted, we can reuse the same object because all subtrees would rerender with the new object and they shouldn't hold a reference to the old one. (Every other render so that the old value can be used in shouldComponentUpdate.)

We lazily attach listeners. Lazily attaching listeners doesn't really help much since these events are often used to determine whether you should do something rather than not listening at all.

It does seem odd that it would have a significant effect. Perhaps it was only in benchmarks.

Someone actually hit this issue yesterday and asked about it. Another hazard is that people defensively clone in each component, leading to more memory thrash than if they could all share the same copy.

It might be worth revisiting. As you said, it makes more sense if more React features adopt pooling as a strategy.

[chenglou]

btw @jordwalke: cloning objects is necessary because of this existing pattern: https://github.com/facebook/react/blob/master/src/browser/ui/__tests__/ReactDOMComponent-test.js#L72

[syranide]

It seems to me like React will go the way of more pooling. Closing this.