Skip to content
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

Direct state in context #26

Merged
merged 10 commits into from
Jun 13, 2019
Merged

Direct state in context #26

merged 10 commits into from
Jun 13, 2019

Conversation

dai-shi
Copy link
Owner

@dai-shi dai-shi commented Jun 13, 2019

As it went well in https://github.com/dai-shi/react-tracked, I'm trying the same technique in reactive-react-redux too.

We put redux state in context and set observedBits = 0.
This should hopefully resolve the concurrent mode issue (but who knows what is concurrent mode...)
Closes #15, unless we further see other concurrent mode issues (in this case file a new issue).

@dai-shi
Copy link
Owner Author

dai-shi commented Jun 13, 2019

2019-06-13 9 51 15

So, it slightly decreases performance especially in "select row".

I'll take a deeper look later. Maybe, it can't be helped.


edit1: 168.2 (+/-) 157.1 seems weird.

edit2: I re-run it. Now, it's moderate. 88.7 (+/-) 7.0 Probably, my computer wasn't stable then.

@dai-shi
Copy link
Owner Author

dai-shi commented Jun 13, 2019

@markerikson You might have some technical interests in this. I'm not sure how react-redux v6 is implemented at code level, but what I'm trying here is to put state in context and still using subscription model. It doesn't seem to gain performance, but hopefully it's concurrent mode friendly.

@markerikson
Copy link

I would generally expect that propagating state via context is going to be a noticeable drop in performance. because that's what we saw with v6.

@dai-shi
Copy link
Owner Author

dai-shi commented Jun 13, 2019

@markerikson It's not that bad. I think it's different from rr v6, in which context propagate all components and HoCs stop updating. Whereas, this one, context doesn't propagate, but calls listeners in batchedUpdates. Probably, the batchedUpdates is important for performance.

@dai-shi
Copy link
Owner Author

dai-shi commented Jun 13, 2019

Probably, the batchedUpdates is important for performance.

This is not correct in this case, because it is in useEffect.

@dai-shi
Copy link
Owner Author

dai-shi commented Jun 13, 2019

Run js-framework-benchmark again. This time, the control is react-redux v7.1.0.

I'm pretty confident that performances are fine.

Only the comparison is:

  • reactive-react-redux v3.0.0 uses store context with unstable_batchedUpdates for correctness and performance
  • reactive-react-redux v4.0.0-alpha.2 uses more or less state context with unstable_calculateChangedBits to avoid context propagation, and it should be concurrent mode friendly

Which is better? A hard question. Anyone have comments? Our hero @gaearon


2019-06-13 21 41 56

@dai-shi
Copy link
Owner Author

dai-shi commented Jun 13, 2019

My intuition is go for it.

I would expect when React would drop the observedBits feature, it should provide other method to bail out useContext.

I would appreciate for any comments even after this PR is merged. Thanks.

@dai-shi dai-shi merged commit 0215fb1 into master Jun 13, 2019
@dai-shi dai-shi deleted the direct-state-in-context branch June 13, 2019 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Concurrent mode?
2 participants