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

Optimize dispatch plain object check #2599

Merged
merged 2 commits into from
Nov 16, 2017
Merged

Optimize dispatch plain object check #2599

merged 2 commits into from
Nov 16, 2017

Conversation

timdorr
Copy link
Member

@timdorr timdorr commented Sep 5, 2017

Closes #2598

This boosts perf by not exhaustively checking every action of it's object-ness. Dispatch is approximately 56x faster. Across a million runs of simple counter reducer:

master: 4690.358ms
nodash: 82.821ms

I also applied the same thing in combineReducers. I believe the check is sufficient, but I'd love a second set of eyes on it.

We also no longer need to bundle any of lodash, so this saves a good number of bytes too! From the UMD build:

26715 master/redux.js
21518 nodash/redux.js
5761 master/redux.min.js
4813 nodash/redux.min.js

@markerikson
Copy link
Contributor

Looks okay at first glance, although the nuances of JS object detection are something I've never had to deal with. Do we have some tests around this scenario?

@markerikson
Copy link
Contributor

I saw an email from a review comment come through, but I'm not seeing that comment as I'm looking at the diff. The email said:

This seems to have the same problem with an old-fashioned isArray implementation. It always fails if the action comes from another window.

Anyone more knowledgeable have thoughts on that?

@timdorr
Copy link
Member Author

timdorr commented Sep 5, 2017

Another alternative to checking constructor is to toString the object and see if that is [object Object].

@milesj
Copy link

milesj commented Sep 5, 2017

Constructor checks should be enough IMO.

@jdalton
Copy link
Contributor

jdalton commented Sep 5, 2017

The isPlainObject check is certainly more robust, but if it's not needed, and a simpler check is doable, more power to ya.

Does anyone know the history of using plain object checks over simpler ones?
It looks like it's used to warn of incorrect usage. Could this be moved to a doc note to avoid the check entirely?

@timdorr
Copy link
Member Author

timdorr commented Sep 5, 2017

@jdalton It's a good safety net for someone that makes a mistake when setting up something like redux-thunk or another middleware. Happens a fair amount and is more friendly to beginners.

@jdalton
Copy link
Contributor

jdalton commented Sep 5, 2017

Happens a fair amount and is more friendly to beginners.

Could it be something that's available in dev builds and removed in production?
Or maybe toggleable with some debug: false redux option?

How are similar debug helpers handled in react?

@timdorr
Copy link
Member Author

timdorr commented Sep 5, 2017

We have some dev-only checks in combineReducers, but not much else. Unfortunately, it's easy to miss a lot of these issues without 100% test coverage, so having "friendlier" errors in production is pretty helpful to our users. Other than this strict check, they are mostly fairly simple and performant enough to warrant leaving it.

AFAIK, React still uses the dev-expression babel plugin and __DEV__ checks in their code.

@markerikson
Copy link
Contributor

Couple more questions:

  1. What scenarios are likely to result in someone dispatching objects from Object.create(null), or from another window?
  2. What checks would be necessary to catch those?

Per 1, I know I've seen several addons that do stuff like serializing actions over postMessage, Electron main-vs-render interop, and similar concepts. I'm not sure if any of those would run into trouble with this change.

@timdorr
Copy link
Member Author

timdorr commented Sep 6, 2017

Object.create(null)

Seems unlikely. Most folks use object literals.

from another window

This I have zero clue about. Not saying that it's not possible, just that I've never done it and don't know the implications. Can someone wire up a demo so we can test it out?

BTW, my company does this in our Chrome extension. We have a background page with the Redux store that is replicated to the frontend content scripts. We do phone calls, so we need a single place to maintain our dialer's state. I'll ask the engineer who built that out.

@@ -26,7 +25,7 @@ function getUnexpectedStateShapeWarningMessage(inputState, reducers, action, une
)
}

if (!isPlainObject(inputState)) {
if (!inputState || inputState.constructor !== Object) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this would work across iframes or multiple realms. Does it? Are we sure that this is a superset of Lodash's true answer rather than a subset or intersection?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this would work across iframes or multiple realms. Does it?

No it wouldn't. Or objects created with Object.create(null)

Copy link
Member Author

@timdorr timdorr Sep 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I know about this problem. Hence the PR instead of just blasting this into master and calling it a day 😄

It's not a use case I'm familiar with, so I'd love some help understanding it and coming up with some examples or even tests for it.

Copy link

@phsantiago phsantiago Sep 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sry for the noob question but why inputState.constructor !== Object instead of typeof inputState === 'object'?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phsantiago Because object type can represent any object new Number or new Array for example. Instance of Object is always plain.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nasreddineskandrani Nope, but type checker will say what's wrong. For IDE fans there is small boilerplate like this

export const DO_ONE_THING = 'DO_ONE_THING';

Copy link

@aminpaks aminpaks Oct 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TrySound Correct me if I'm wrong, according to you:

It means it's not serializable -> may have side effects -> not predictable. And maybe redux devtools won't work too :)

If this won't break anything, developer should be able to bypass it, no?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TrySound
please read enum string introduced in ts2.4 section here: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-4.html
enum string => give you intellisense ability (save time) and type checking.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aminpaks , @nasreddineskandrani , @TrySound : I do appreciate the enthusiasm and interest, but this discussion would probably be better off in a different location.

The Reactiflux chat channels at https://www.reactiflux.com always have plenty of people around who are happy to discuss Redux-related topics. There's somewhat less discussion of TypeScript or NgRX, but there might be some people who can discuss those.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markerikson I hesitate to comment but since the code was in a sub thread. I gived my 2 cents. Thanks for the link I ll be happy to exchange about redux their.

@gaearon
Copy link
Contributor

gaearon commented Sep 6, 2017

My two cents:

  1. Cross-iframe and cross-realm things can get weird and cause false negatives. I'm really happy with the effort that went into Lodash implementation.

  2. I don't think benchmarks are very relevant. If you're dispatching more than one or two actions in a second you already have a performance problem because your subscribers are executing much more often. You might want to reconsider using Redux if dispatching is your bottleneck.

@timdorr timdorr changed the title Remove lodash. Optimize dispatch plain object check Sep 6, 2017
@timdorr
Copy link
Member Author

timdorr commented Sep 6, 2017

<Retitling this so it doesn't come across as a dig against lodash>

@markerikson
Copy link
Contributor

More research questions:

  1. What edge cases do all the individual checks in lodash.isPlainObject actually handle?
  2. Which of those checks are the most expensive?
  3. Which of the edge cases are we actually concerned about in practice?

@markerikson
Copy link
Contributor

@gaearon , I've anecdotally seen plenty of cases where people are firing off way more than "one or two actions a second". I agree that subscribers are generally the bottleneck, but if we can reasonably say that there's some "plain object" edge cases that we aren't concerned with and improve dispatching perf overall, it's worth at least investigating.

@gaearon
Copy link
Contributor

gaearon commented Sep 6, 2017

The problem is that we'll likely get false negatives for plain objects rather than false positives. In other words we'll now throw for things that are actually completely okay (and are plain objects).

This is hugely disruptive to people who already rely on current behavior in their apps, sometimes without a good alternative.

I've anecdotally seen plenty of cases where people are firing off way more than "one or two actions a second"

OK. Let's say 60 dispatches per second is the maximum I would consider reasonable. Does this PR make a measurable difference in this case?

@rodrigorm
Copy link

@gaearon I have run a sample with 60 dispatches before and apter applying the patch:

$ node .
master: 0.164ms
nodash: 0.162ms

@hasyee
Copy link

hasyee commented Sep 6, 2017

@timdorr @gaearon I can suggest a lazy assertion.

  1. first we check the action by my previous less strict suggestion
  2. if it does not meet with the first restriction, we can check with isPlainObject or something else

Benefits: we do not punish the regular users, but in edge cases there may be performance overheads

IMHO I think we should leave Lodash, because that's a heavy dependency.
My new suggestion:

if (!action || (action.constructor !== Object && !action.__proto__ && typeof action !== 'object')) {
  throw new Error('Actions must be plain objects. ' + 'Use custom middleware for async actions.');
}

@gaearon
Copy link
Contributor

gaearon commented Sep 6, 2017

I like this (idea, not the code snippet which I think doesn't do that). Any downsides?

@gaearon
Copy link
Contributor

gaearon commented Sep 6, 2017

Are there objects with constructor === Object but that are not considered plain objects by Lodash?

@hasyee
Copy link

hasyee commented Sep 6, 2017

I can imagine some frequently used dispatch: logging. I've used Redux for this, and I was glad by its performance. I think the performance improving is a cardinal purpose, because Redux is a very essential library in the most of React projects. If we can make it smaller and faster, that can only be good.

@hasyee
Copy link

hasyee commented Sep 6, 2017

@gaearon I tried my snippet with regular and Object.create(null) actions. Both of them worked.
With regular actions I measured 70-90 ms.
With the other that was 200-300 ms.
I am not sure that covers all of cases that isPlainObject performs. But I can agree, that the lazy assertion is our first approximation, and removing Lodash may be an other issue.

Idea: we should use Lodash's test cases.

@gaearon
Copy link
Contributor

gaearon commented Sep 6, 2017

What I'm trying to say is whatever the cost of isPlainObject is, the cost of calling subscribers and reducers will likely dwarf it in real projects. Can we please test this on a real project with 60 dispatches per second before proceeding?

@jimbolla
Copy link
Contributor

jimbolla commented Sep 6, 2017

react-redux also uses lodash's isPlainObject to test the results of mapXToProps. It only does it during dev builds, not production. It seems like the intent of this code is the same; to make sure the developer isn't doing anything wacky. By the time it gets to prod build, the issue should have been resolved.

@lukescott
Copy link

lukescott commented Oct 26, 2017

I think object.constructor === Object would only be an issue with cross-frames if:

  • The frame is the same origin
  • You compare a variable from one frame to another: An Object in one frame is different from an Object in another.

But if you're using postMessage, the communication is serialized, so it shouldn't be an issue. Typically taking a variable from another frame is a bad idea in general.

@timdorr timdorr added this to the 4.0 milestone Nov 3, 2017
@timdorr timdorr changed the base branch from master to next November 3, 2017 17:30
@timdorr
Copy link
Member Author

timdorr commented Nov 3, 2017

I'm going to make this held off for 4.0, so we can get more extensive testing via the prerelease cycle. It's now rebased against the next branch.

Just to be clear, I'm on the same side of not seeing this being about a speed optimization, as dispatches should be relatively infrequent in most applications and not a bottleneck. Instead, it's a good chance to drop an external dependency that brings a fair amount of bytes with it. It's not huge, but every little bit helps.

But the key blocker here is equivalency of the plain object check. I'd love a simple function for doing that. What would be most helpful here is a test harness I can use to ensure this is equivalent. Something around message passing via iframes and fun edge case stuff. I think we can do it 👍

matthargett added a commit to matthargett/react-redux that referenced this pull request Apr 24, 2018
timdorr pushed a commit to reduxjs/react-redux that referenced this pull request May 31, 2018
* Simplify object check and eliminate lodash depdency, mirroring reduxjs/redux#2599 .

* Add missing files.

* Replace isPlainObject with unmodified version from Redux

* Also replace the spec with the unmodified version
josepot pushed a commit to josepot/react-redux-lean that referenced this pull request Sep 21, 2018
* Simplify object check and eliminate lodash depdency, mirroring reduxjs/redux#2599 .

* Add missing files.

* Replace isPlainObject with unmodified version from Redux

* Also replace the spec with the unmodified version
toptaldev92 pushed a commit to toptaldev92/react-redux that referenced this pull request Jul 28, 2021
* Simplify object check and eliminate lodash depdency, mirroring reduxjs/redux#2599 .

* Add missing files.

* Replace isPlainObject with unmodified version from Redux

* Also replace the spec with the unmodified version
kronooss added a commit to kronooss/coin-trading that referenced this pull request Aug 22, 2022
* Simplify object check and eliminate lodash depdency, mirroring reduxjs/redux#2599 .

* Add missing files.

* Replace isPlainObject with unmodified version from Redux

* Also replace the spec with the unmodified version
sven971105 added a commit to sven971105/react-redux that referenced this pull request Sep 21, 2022
* Simplify object check and eliminate lodash depdency, mirroring reduxjs/redux#2599 .

* Add missing files.

* Replace isPlainObject with unmodified version from Redux

* Also replace the spec with the unmodified version
@phryneas phryneas mentioned this pull request Apr 1, 2023
charmingdev222 pushed a commit to charmingdev222/react-redux-demo that referenced this pull request May 16, 2023
* Simplify object check and eliminate lodash depdency, mirroring reduxjs/redux#2599 .

* Add missing files.

* Replace isPlainObject with unmodified version from Redux

* Also replace the spec with the unmodified version
ZinedineDumas pushed a commit to harry908nilson/edux that referenced this pull request Aug 4, 2023
* Simplify object check and eliminate lodash depdency, mirroring reduxjs/redux#2599 .

* Add missing files.

* Replace isPlainObject with unmodified version from Redux

* Also replace the spec with the unmodified version
MiyamotoSatoshi added a commit to MiyamotoSatoshi/redux that referenced this pull request Aug 15, 2023
* Simplify object check and eliminate lodash depdency, mirroring reduxjs/redux#2599 .

* Add missing files.

* Replace isPlainObject with unmodified version from Redux

* Also replace the spec with the unmodified version
peace19920726 added a commit to peace19920726/React-Redux-Dev that referenced this pull request Sep 1, 2023
* Simplify object check and eliminate lodash depdency, mirroring reduxjs/redux#2599 .

* Add missing files.

* Replace isPlainObject with unmodified version from Redux

* Also replace the spec with the unmodified version
Bee1130 added a commit to Bee1130/React-Redux that referenced this pull request Jan 19, 2024
* Simplify object check and eliminate lodash depdency, mirroring reduxjs/redux#2599 .

* Add missing files.

* Replace isPlainObject with unmodified version from Redux

* Also replace the spec with the unmodified version
fairsky0201 added a commit to fairsky0201/react-redux that referenced this pull request Apr 17, 2024
* Simplify object check and eliminate lodash depdency, mirroring reduxjs/redux#2599 .

* Add missing files.

* Replace isPlainObject with unmodified version from Redux

* Also replace the spec with the unmodified version
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.