Using lodash.isPlainObject in dispatch has performance overhead

[hasyee]

I just created a performance comparison between Redux and Repatch: https://github.com/jaystack/redux-repatch-performance-comparison.

This comparison dispatches a big amount of pure actions (simple increment), and I found that Redux is too slow as we expect.

I discovered, that this assertion at dispatch causes this performance decreasing.

I suggest a simpler checking, like:

action && action.constructor === Object

if it's not necessary that action objects can made by Object.create(null).

With this enhancing redux dispatching will be 2 magnitude faster.

[timdorr]

That certainly looks to be the case:

⚡️  ~/src/redux-repatch-performance-comparison (⭠ master|✔) $ node index.js
redux: 4576.211ms
repatch: 44.051ms
⚡️  ~/src/redux-repatch-performance-comparison (⭠ master|✔) $ node index.js
redux: 88.650ms
repatch: 44.394ms

On the second run I just remove the safety check entirely.

Now, the question is do we need that level of accuracy and compatibility in our code. I would argue no, that a simple constructor check should be sufficient for 99.9% of users. But do others have some thoughts on this subject?

[markerikson]

Huh. Yeah, I tried running your benchmark yesterday. I was a bit skeptical of the "9x difference" claim, but the numbers bore it out, and the store setup seemed very fair.

Now, in practice, my assumption is that this is not going to be the real bottleneck. Everything I've seen on perf so far says that the majority of the overhead comes from notifying subscribers and updating the UI, and a microbenchmark of dispatching 1M actions nonstop is not the most realistic use case.

However, if we can come up with a tweak to the check that speeds things up without breaking anything, I'm on board.

[Rokt33r]

I think this assertion is not necessary on production. Why don't we do it on development env only?
Is there any case exploiting this in real world?

[timdorr]

@Rokt33r It's most to prevent failures later on in the code that are less self-explanatory (e.g., "Cannot read property something of undefined").

[Rokt33r]

I see. I just noticed that I've never met the problem because I've been always using typescript.

[drcmda]

@markerikson in our case dispatches fire so often, we did notice how it can nag at the applications fps, even when actual ui delegation is debounced/async/requestIdleFrame. It made us look into ways to combine many calls into one, but haven't gotten that far. To lessen the effect of dispatch would be very welcome. 😃

[markerikson]

@drcmda : yeah, I wasn't saying this would be a pointless change, just that for most people it's not likely to have a huge perf impact.

For batching, there's many options available. You might want to look at the Store#Batching section of my Redux addons catalog to see what's available.

As a side note, I've seen a lot of comments from you on Reddit about the kind of app your team is building. At some point I'd be interested in having a longer discussion about how your team is using Redux.

[hasyee]

Does anyone know at all that store.disaptch accepts actions made by Object.create(null)? To leave supporting this, we can replace the assertion, and I guess it won't break for the 99.99% of users.

[timdorr]

@jayhasyee I don't see any reason why not, but it would be extremely rare. Most folks are using object literals.

[sophister]

hi, why not just using the following code to check plain object?

function isPlainObject(obj){
    return Object.prototype.toString.call(obj) === '[object Object]';
}