Optimize dispatch plain object check

[timdorr]

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]

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]

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]

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

[milesj]

Constructor checks should be enough IMO.

[jdalton]

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]

@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]

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]

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]

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]

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.

[gaearon]

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?

[jdalton]

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)

[timdorr]

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.

[phsantiago]

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

[TrySound]

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

[TrySound]

@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';

[aminpaks]

@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?

[nasreddineskandrani]

@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.

[markerikson]

@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.

[nasreddineskandrani]

@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]

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]

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

[markerikson]

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]

@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]

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]

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

$ node .
master: 0.164ms
nodash: 0.162ms

[hasyee]

@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]

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

[gaearon]

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

[hasyee]

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]

@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]

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]

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]

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]

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 👍