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

Fixed "Maximum call stack size exceeded" #311

Merged
merged 2 commits into from
Aug 29, 2019

Conversation

gnought
Copy link
Collaborator

@gnought gnought commented Aug 26, 2019

Attempt to fix "Maximum call stack size exceeded" …
Reported in #193
This could be reproduced using benchmark/server.js, benchmark/throughputCounter.js and benchmark/bombing.js
Once all three processes are up, stay a while and SIGINT the throughtputCounter.js

Reported in moscajs#193
This could be reproduced using benchmark/server.js, benchmark/throughputCounter.js and benchmark/bombing.js
Once all three processes are up, stay a while and SIGINT the throughtputCounter.js
Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

One of the key things to get a good performance out of Node.js is to not use setImmediate in this manner. Can we avoid that?

@gnought
Copy link
Collaborator Author

gnought commented Aug 26, 2019

Yes, i know the impact of setImmediate, but we need to let nodejs to do something for free up the stack.

@mcollina
Copy link
Collaborator

An hacky but effective way to do that is to maintain a syncCounter module-level variable. We increment when we enter that method, and decrement afterwards. If it's over XX, we wrap the callback in a nextTick.

@gnought
Copy link
Collaborator Author

gnought commented Aug 26, 2019

can do, what is the best value of the max syncCounter ?

@mcollina
Copy link
Collaborator

Try various numbers until your test pass.

@gnought
Copy link
Collaborator Author

gnought commented Aug 28, 2019

@mcollina we got some interesting result. Using setImmediate() is faster than without setImmedate().

no setImmediate():

node benchmarks/throughputCounter.js
received/s 0
received/s 53530.6
received/s 76027
received/s 74329.8
received/s 73931.6
received/s 74014
received/s 73337.40000000001
received/s 73272.2
received/s 74421
received/s 73559.6
received/s 70127.2

node benchmarks/bombing.js
sent/s 88457.8
sent/s 75371.4
sent/s 76037.59999999999
sent/s 73301.8
sent/s 73167.4
sent/s 73167.2
sent/s 74584
sent/s 74125.4
sent/s 70792.8
sent/s 72399.4

Using setImmediate()

node benchmarks/throughputCounter.js
received/s 0
received/s 54378.799999999996
received/s 84528.2
received/s 91489.40000000001
received/s 91082
received/s 90599.8
received/s 90379.40000000001
received/s 92739.8
received/s 93722.6
received/s 87883

node benchmarks/bombing.js
sent/s 85745.40000000001
sent/s 84512.4
sent/s 91967.4
sent/s 91144.59999999999
sent/s 90429.8
sent/s 91892.6
sent/s 92239
sent/s 92946.20000000001
sent/s 86141.2

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@mcollina mcollina merged commit d7a1f82 into moscajs:master Aug 29, 2019
@gnought gnought deleted the hotfix/fix_max_stack_size branch August 30, 2019 14:39
@gnought gnought added the bug label Aug 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants