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

Sinon stub with an object and a property has low performance. #1627

Closed
NoamPaz opened this issue Dec 7, 2017 · 5 comments
Closed

Sinon stub with an object and a property has low performance. #1627

NoamPaz opened this issue Dec 7, 2017 · 5 comments
Labels

Comments

@NoamPaz
Copy link
Contributor

NoamPaz commented Dec 7, 2017

Sinon stub with an object and a property name has low performance due to new Error in the wrap-method.js.

When I create stub with sandbox.stub and pass an object and a property name, it reach this line in the wrap-method.js:
method.stackTrace = (new Error("Stack Trace for original")).stack;

https://github.com/sinonjs/sinon/blob/master/lib/sinon/util/core/wrap-method.js#L110

This line takes about 0.2-0.3 second, therefore my tests runs for 6 minutes.
If I comment it out, it runs for 7 seconds (instead of 6 minutes).

  1. I'm using the latest version of sinon.
  2. I tried to use node v6 and node v8. in both the issue has occurred.
  3. I'm using Linux.
  4. here is an example of the stack trace this new Error generates:

Error: Stack Trace for original
at wrapMethod (/home/noam/t2kdev/echo/node_modules/sinon/lib/sinon/util/core/wrap-method.js:110:26)
at stub (/home/noam/t2kdev/echo/node_modules/sinon/lib/sinon/stub.js:60:44)
at Object.stub (/home/noam/t2kdev/echo/node_modules/sinon/lib/sinon/collection.js:102:33)
at global.createStub (/home/noam/t2kdev/echo/.tmp/mocha-webpack/1512641354923/webpack:/node_modules/@t2k/t2k-js-scripts/test-setup.js:33:1)
at Context. (/home/noam/t2kdev/echo/.tmp/mocha-webpack/1512641354923/webpack:/test/spec/js/components/App.spec.jsx:208:4)
at callFnAsync (/home/noam/t2kdev/echo/node_modules/mocha/lib/runnable.js:371:21)
at Hook.Runnable.run (/home/noam/t2kdev/echo/node_modules/mocha/lib/runnable.js:318:7)
at next (/home/noam/t2kdev/echo/node_modules/mocha/lib/runner.js:309:10)
at Immediate. (/home/noam/t2kdev/echo/node_modules/mocha/lib/runner.js:339:5)
at runCallback (timers.js:672:20)
at tryOnImmediate (timers.js:645:5)
at processImmediate [as _immediateCallback] (timers.js:617:5)

@fatso83
Copy link
Contributor

fatso83 commented Dec 7, 2017

Whoa! 0.2 seconds! Can you make a reproducible test case for us that demonstrates this? I have never been close to anything like that. More like 0.02ms. Maybe you have an enourmous stack depth or something? It doesn't look like it, but ...

@mroderick
Copy link
Member

Interesting issue!

@NoamPaz
Copy link
Contributor Author

NoamPaz commented Dec 9, 2017

The slowness seems to come from accessing the .stack property on the error, as v8 seems to go through considerable trouble in order to convert it from its internal representation to the string version it uses to report errors.

IMO the best fix for this would be to change the line to method.errorForStackTrace = new Error("Stack Trace for original"), and only access the .stack property if an error needs to be reported.

Here is a demonstration:
https://gist.github.com/NoamPaz/3728639389e5caa095ef05418f076cd9
In this demonstration the stack is very short, the performance differences will be much more noticeable for a long stack.

@fatso83
Copy link
Contributor

fatso83 commented Dec 11, 2017

Ok, if I find time (haha) I'll try to reproduce this. Hoping for someone else to do it though, along with an automatic test for pre/post performance.

We could increase the stack depth easily through recursion, say 65000 times, in such a test case.

NoamPaz added a commit to NoamPaz/sinon that referenced this issue Jan 23, 2018
Access the .stack property if an error needs to be reported.
@NoamPaz NoamPaz mentioned this issue Jan 23, 2018
mroderick pushed a commit that referenced this issue Jan 23, 2018
Access the .stack property if an error needs to be reported.
@mroderick
Copy link
Member

This has been fixed by #1671 and published as [email protected]

franck-romano pushed a commit to franck-romano/sinon that referenced this issue Oct 1, 2019
Access the .stack property if an error needs to be reported.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants