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

Fix to #1627 #1671

Merged
merged 1 commit into from
Jan 23, 2018
Merged

Fix to #1627 #1671

merged 1 commit into from
Jan 23, 2018

Conversation

NoamPaz
Copy link
Contributor

@NoamPaz NoamPaz commented Jan 23, 2018

Fix issue #1627 by creating an error object and use the property 'stack' only when needed, instead of read stack property in the creation time of the error object.

Background (Problem in detail)

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.

Solution

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

####Test
passed: 'npm run test' and 'npm run lint'

Access the .stack property if an error needs to be reported.
@NoamPaz NoamPaz changed the title #1627 Fix to #1627 Jan 23, 2018
@mroderick
Copy link
Member

Thank you for taking the time to investigate this issue and making a pull request 🚀

Your example script in the gist doesn't exercise actual Sinon code. I wanted to make sure that there would be an improvement for an example that exercises Sinon.

So, I created a little benchmark script using Benchmark.js.

"use strict";

var Benchmark = require("benchmark");
var suite = new Benchmark.Suite();
var sinon = require("./lib/sinon");
var sandbox = sinon.createSandbox();

// add tests
suite.add("stub test", function () {
    var obj = {
        hello: function hello() {
            return "world";
        }
    };

    sandbox.stub(obj, "hello");
});
// add listeners
suite.on("cycle", function (event) {
    console.log(String(event.target));
});
suite.on("complete", function () {
    console.log("Fastest is " + this.filter("fastest").map("name"));
});
// run async
suite.run({
    "async": true
});

When run against current master 5063c33, these are the results on my ageing laptop:

stub test x 4,852 ops/sec ±3.78% (59 runs sampled)

Run against this branch:

stub test x 10,129 ops/sec ±3.17% (64 runs sampled)

The benchmarks show a pretty significant speedup!

Excellent pull request 👍

@mroderick mroderick merged commit a5c22f2 into sinonjs:master Jan 23, 2018
@mroderick
Copy link
Member

This has been published as sinon@4.2.1

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

Successfully merging this pull request may close these issues.

2 participants