Remove usage of PooledClass

[PezCoder]

In PooledClass.js we have some static poolers based on the number of arguments & the comment specifies it's not made dynamic so we don't have to use arguments.

I have 2 questions with this:

1. What is the issue with using arguments (i noticed we're using it in other place reference) if it's going to reduce duplicate code ?
2. Can't we use the spread syntax for this like this:

function nArgumentPooler(...args) {
    var Klass = this;
    if (Klass.instancePool.length) {
        var instance = Klass.instancePool.pop();
        Klass.call(instance, args);
    	return instance;
    } else {
        return new Klass(args);
    }
}

[gaearon]

The person who wrote this code several years ago was likely worried about JS engine deoptimizations caused by using arguments.

To be honest I think the whole PooledClass abstraction needs to go. It might not be as necessary anymore with modern browsers. We discussed this before, and I think we haven’t really reached a decision there.

Would you like to send a PR to remove it and rewrite the code that relies on it? We could then experiment with the result and see its effect on performance.

[aweary]

It's worth noting that while modern browsers may optimize arguments access, we support some older browsers, so we would need to profile performance with those as well.

It's not a big deal if there is a small performance regression in a limited number of limited use browsers, but anything more would probably be worth considering more carefully.

[nhunzaker]

@aweary how would we go about doing that profiling? Is that something we could add to our fixture data? Or should that go somewhere else?

[aweary]

Maybe, but it would be less clear what was passing and failing behavior. I imagine you'd have to write an app that taxed PooledClass (something that allocates a lot of SyntheticEvents maybe) and then just compare performance before and after the changes.

[gaearon]

traverseAllChildren used by ReactChildren also uses pooling so maybe that could be used in a benchmark instead.

It's worth noting that while modern browsers may optimize arguments access, we support some older browsers, so we would need to profile performance with those as well.

The whole issue with arguments is moot if we remove PooledClass altogether. To be clear, I'm suggesting that rather than changing the pooling code as in the first comment. Also, in general we're okay with slight performance regressions in older browsers if it helps drop bundle size.

[aweary]

Okay, that works. As far as I know, object pooling is done to limit object allocations when possible. In that case, we should benchmark on low-memory devices to see if removing PooledClass negatively affects performance there.

[gaearon]

I'm not really sure what kind of benchmark could show it. traverseAllChildren was used in hot paths in Stack, but Fiber doesn't use it for reconciliation anymore. So we're left with React.Children using it (relatively uncommon path), and SyntheticEvent (would you really get so many events per second to cause GC trash?) I can imagine this being meaningful on React Native though. For low end devices on the web, let's also keep in mind shipping more bytes means slower init time.

[gaearon]

My proposal: let's remove pooling from traverseAllChildren since it's not a hot path in Fiber. Then let's rewrite the event pooling code by hand specifically for that case (it should be possible to make it shorter I hope). Anyone wants to try?

[aweary]

I can handle it unless someone else really wants to take it 😃

[gaearon]

Renamed the issue to be more focused. Thanks @aweary!

[koba04]

@aweary @gaearon If you consider removing usage of PooledClass from SyntheticEvent, I can work on to investigate whether PooledClass is needed for SyntheticEvent or not.

I have a work in progress branch for this.

https://github.com/koba04/react/tree/remove-pooling-synthetic-event

A commit is koba04@2ef294f
(All tests passes)

[syranide]

Regarding SyntheticEvents, I challenged it some time ago and the team was insisting that it was a necessary gain (even considering the API impact), especially as they had witness "GC thrashing" in even modern browsers. (some info; #1612) It may have changed since then though.

But like I suggested elsewhere, multiple events of the same type are never really in-flight at the same time, so just keeping a single event should be enough, pooling should not be necessary. #1664 (comment)

[gaearon]

The approach I would like to try is to not use pooling for traversal, but to hardcode pooling implementation into the event system with less dynamic indirection. I think it would make sense to start with a single event object per type since each type might have different fields. We can later look if we can reduce it to one object overall.

[koba04]

I've picked files depending on PooledClass.

The following files are dependencies of Stack reconciler.
So it seems to be able to remove them when Fiber is landed (in v16?)

• ReactReconcileTransaction
• ReactServerRenderingTransaction
• CallbackQueue
• ReactUpdates
• ReactTestReconcileTransaction

To remove PooledClass, we would need to consider about the following files.

• FallbackCompositionState
• ReactEventListener
• ReactNativeReconcileTransaction
• SyntheticEvent
• ReactChildren (PR Reimplement React.Children #9339)

[aweary]

@gaearon how should we handle removing PooledClass with the stack renderer still utilizing it?

[aweary]

ping @gaearon, hoping you could provide some more guidance on ^

[gaearon]

I’m mostly interested in looking at removing its occurrences in Fiber. Stack would be deleted soon anyway.

[aweary]

@gaearon so in that case, it probably doesn't make sense to go through and remove PooledClass from stack-specific code paths. I suppose that depends on how many more 15 releases we intend to cut and whether making those changes would be worth the risk.

[gaearon]

Oh, we definitely don't want to do this in 15.x so you're right.

[PezCoder]

Posted this issue in search for a good first bug but I started a beast here.
This is what happens when you're trying to improve one thing & later doubt the existence of the whole thing 😉

[gaearon]

Haha, sorry! I think we can close this because we removed its usage in the modern part of the codebase, and we’ll delete the legacy one very soon.

[282159468]

The person who wrote this code several years ago was likely worried about JS engine deoptimizations caused by using arguments.

To be honest I think the whole PooledClass abstraction needs to go. It might not be as necessary anymore with modern browsers. We discussed this before, and I think we haven’t really reached a decision there.

Would you like to send a PR to remove it and rewrite the code that relies on it? We could then experiment with the result and see its effect on performance.

I am too low in technology and have too many questions, but I want to know why

1. I found that react@16.13.0 deleted the pooledclass.js file, but in the following three files I found a separate implementation similar to pooledclass. What is the reason for this?
   
2. why JS engine deoptimizations caused by using arguments.
   i get https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#3-managing-arguments
3. why It might not be as necessary anymore with modern browsers