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

Usage with redux router spikes CPU to 100% #23

Closed
kolodny opened this issue Nov 30, 2015 · 18 comments
Closed

Usage with redux router spikes CPU to 100% #23

kolodny opened this issue Nov 30, 2015 · 18 comments

Comments

@kolodny
Copy link

kolodny commented Nov 30, 2015

I've narrowed my issue down to this triggering it:

const finalCreateStoreArgs = [
  applyMiddleware(...middleware),
  //reduxReactRouter({ createHistory }), // <-- runs find when commented
  //devTools(),
  global.devToolsExtension ? global.devToolsExtension() : f => f,
];

// vs

const finalCreateStoreArgs = [
  applyMiddleware(...middleware),
  reduxReactRouter({ createHistory }), // <--- CPU spikes to 100%
  //devTools(),
  global.devToolsExtension ? global.devToolsExtension() : f => f,
];
@zalmoxisus
Copy link
Owner

Hey @kolodny,

Tried here and don't see any problems, even without any delay. Could you please provide more details, an example to test or try to compare it with my boilerplate?

@kolodny
Copy link
Author

kolodny commented Dec 1, 2015

Looking into it, I profiled the page and it appears there's one function that seems to be the bottleneck:
image

which takes me here
image

which looks like it's coming from here https://github.com/zalmoxisus/redux-devtools-extension/blob/1d177edf39eaf6d00dc96a58269cfdc03252780b/src/browser/extension/inject/pageScript.js

@kolodny
Copy link
Author

kolodny commented Dec 1, 2015

I haven't been able to show a proof of work, the code I have is pretty customized now

@zalmoxisus
Copy link
Owner

Thanks for detailed info. That code is from json-stringify-safe. Though it is not the problem, but just the consequence, you can try to disable the serialization, as described here, and see if the extension works. Also there in the Options page, you may add a bigger delay so it will do the serialization and message posting not so often.

The problem is that you got somehow an endless number of changes in the Devtools store (liftedStore), and we still don't know the cause. If you could help me to reproduce it, I'd can help with solving it. It would be great if you'd try to run my boilerplate, it contains redux router and hmr as well, just versions of some modules may be different. If it is possible to add some modifications to it to reproduce the issue.

@zalmoxisus
Copy link
Owner

Hey @kolodny,

Still wasn't able to replicate this issue, but I added the ability to filter actions in v0.4.5, so you may hide (exclude from being processing) specific actions or catch the action causing an infinite loop by adding different actions in the "Actions to show" field.
screen shot 2015-12-04 at 1 51 43 pm

@kolodny
Copy link
Author

kolodny commented Dec 4, 2015

Nice, I'm gonna try to start binary adding my components to the boilerplate
to try to replicate
On Dec 4, 2015 6:52 AM, "Mihail Diordiev" notifications@github.com wrote:

Hey @kolodny https://github.com/kolodny,

Still wasn't able to replicate this issue, but I added the ability to
filter actions in v0.4.5, so you may hide (exclude from being processing)
specific actions or catch the action causing an infinite loop by adding
different actions in the "Actions to show" field.
[image: screen shot 2015-12-04 at 1 51 43 pm]
https://cloud.githubusercontent.com/assets/7957859/11589090/3b29be68-9a8e-11e5-8238-6ac467c8b5e3.png


Reply to this email directly or view it on GitHub
#23 (comment)
.

@zalmoxisus
Copy link
Owner

I hope the issue was related to json-stringify-safe (as seen in logs), if so, it should be solved in v.0.4.9 using circular-json as described there.

Please let me know if it still occurs. There's a similar issue with persistStore (even in the regular Redux DevTools). If we manage to solve this one, the latter could be solved as well.

@kolodny
Copy link
Author

kolodny commented Dec 17, 2015

I'm seeing a huge CPU spike on page load in my app. I haven't really had
any time to investigate yet :(

On Thu, Dec 17, 2015 at 10:22 AM, Mihail Diordiev notifications@github.com
wrote:

I hope the issue was related to json-stringify-safe (as seen in logs), if
so, it should be solved in v.0.4.9 using circular-json as described there
https://github.com/WebReflection/circular-json#why-not-the-izs-one.

Please let me know if it still occurs. There's a similar issue with
persistStore (even in the regular Redux DevTools). If we manage to solve
this one, the latter could be solved as well.


Reply to this email directly or view it on GitHub
#23 (comment)
.

@zalmoxisus
Copy link
Owner

Thank you @kolodny for your reports, it was helpful a lot. It seems to me that there are no related bugs caused by json-stringify-safe or redux-router, it's just about the serialization being expensive. The problem appears with Redux Router because it has circular references, without them we don't have to serialize anything.

I added an example with Redux Router, where we generate consecutive actions ("AutoTodo") in order to simulate high activity. So, yes, with more than 2000 tasks in todo I see serialization being a huge bottleneck:
screen shot 2015-12-19 at 10 19 05 am

A temporary solution would be to increase the delay (as described here). With a delay of 10 seconds, even with a huge state object it works smoothly:
screen shot 2015-12-19 at 10 20 02 am

I'll try to find a way to relay only changes not the whole lifted store, I guess I'll had to rewrite the Redux Devtools implementation.

@gaearon
Copy link

gaearon commented Dec 19, 2015

I heard Redux Router puts components into state so this may be the problem.
This problem shouldn't exist with https://github.com/jlongster/redux-simple-router I think.

@zalmoxisus
Copy link
Owner

@gaearon, yes, redux-simple-router has no circular references and we don't even need to serialize anything.

By the way, there's the same problem as here with persistState (for regular Redux DevTools), it wouldn't work with Redux Router as JSON.stringify doesn't support circular references (that Redux Router uses). I was thinking to add a pull request to fix the serialization, but it turns out that workarounds like json-stringify-safe or circular-json aren't performant enough.

zalmoxisus added a commit that referenced this issue Dec 23, 2015
@zalmoxisus
Copy link
Owner

Now (in v0.4.12) we relay actions and the current state instead of the whole lifted store state (history tree). Tested the example above, and there's no signifiant cpu impact of the serialization anymore.

When you have some time to test it, please let me know whether it works well for you now. If still something goes wrong, feel free to reopen the issue.

@kolodny
Copy link
Author

kolodny commented Dec 27, 2015

Just checked, it seems to work great now, Thanks!

@kolodny
Copy link
Author

kolodny commented Dec 28, 2015

btw I wrote a library for serializing and deserializing json that can have circular refs and is pretty performant, not sure if it matters anymore tho https://github.com/kolodny/jsan

@zalmoxisus
Copy link
Owner

Of course it matters, there's always space for optimisations. The library is awesome! As states shouldn't be mutated, we will not have non-circular self references inside. Am I right, @kolodny? Could we rely on the default jsan.stringify or should we force it to handle those cases?

@kolodny
Copy link
Author

kolodny commented Dec 29, 2015

If the only types of references you will have are circular then jsan is what you're looking for. That's the exact use case that it really shines and here why:

The basic way it works is that there are two ways to call stringify, the regular way jsan.stringify(obj) and the forced traversal way jsan.stringify(obj, null, null, true).

The regular way which is as fast as JSON.stringify for objects without circular references is tried, if that works then it will return that result. If it fails, and the only way it can fail is because of circular references, then it will move onto traversal mode which handles circular references (as well as dates and self references)

On the parse side, if the inputted string contains a special marker, which will only be there if it used the traversal method on stringify, then it will use a traversal method to reconnect the object, otherwise it will use the native JSON.parse

A lot of work has gone into getting every last bit of performance, things like avoiding .apply and even calling JSON.parse and JSON.stringify with a single argument since it was shown to be faster

@zalmoxisus
Copy link
Owner

Thanks a lot for the detailed explanation! Will use it here and for remote-redux-devtools as well.

@kolodny
Copy link
Author

kolodny commented Dec 29, 2015

👍

zalmoxisus added a commit that referenced this issue Dec 30, 2015
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

No branches or pull requests

3 participants