Skip to content

drop clone module in favor of JSON.parse & JSON.stringify combo for better performance - #3

Merged
simov merged 3 commits into
request:masterfrom
ahmadnassri:use-json-stringify
May 20, 2015
Merged

drop clone module in favor of JSON.parse & JSON.stringify combo for better performance#3
simov merged 3 commits into
request:masterfrom
ahmadnassri:use-json-stringify

Conversation

@ahmadnassri

Copy link
Copy Markdown

benchmarks show JSON.stringify to be faster and uses less memory

  JSON.stringify   x 40,423 ops/sec ±1.77% (87 runs sampled)
  clone            x 17,677 ops/sec ±2.36% (93 runs sampled)

@FredKSchott

Copy link
Copy Markdown

Random thought: If headers is just a collection of strings, using Object.assign() could be even faster.

@ahmadnassri

Copy link
Copy Markdown
Author

@FredKSchott correct, that's ES6 though .. this library probably needs to have backward compatibility with early node versions, and doing polyfills goes back to performance issues.

@FredKSchott

Copy link
Copy Markdown

Ah good point, I didn't even think about how the polyfill would behave, performance wise.

@simov

simov commented May 1, 2015

Copy link
Copy Markdown
Member

@ahmadnassri 👍 from me, I even added two more modules to your benchmarks here ahmadnassri/benchmark-node-clone#1

@eiriksm

eiriksm commented May 1, 2015

Copy link
Copy Markdown
Member

Couldn't we make it so that we still keep the clone() function calls and instead just redefined

var clone = function(obj) {
  return JSON.parse(JSON.stringify(obj));
};

Just in case we in the future would want to swap it for Object.assign or something. Minor thing, of course.

@ahmadnassri

Copy link
Copy Markdown
Author

@eiriksm I couldn't resist creating a wrapper: https://github.com/ahmadnassri/stringify-clone

more for code cleanliness really ... I don't like seeing JSON.parse(JSON.stringify(obj)) everywhere ...

@eiriksm

eiriksm commented May 1, 2015

Copy link
Copy Markdown
Member

Looks good except you put the wrapper as devDependency when it really is dependency

@ahmadnassri

Copy link
Copy Markdown
Author

@eiriksm ah, thanks for catching that!

@ahmadnassri

Copy link
Copy Markdown
Author

travis build was failing due to node 0.8 missing dependency in stringify-clone this has been fixed, run the travis build again, and it should pass.

@simov

simov commented May 5, 2015

Copy link
Copy Markdown
Member

The tests are still failing on node 0.8 even though the engine in stringify-clone is set to >=0.8, @nylen can we update the support to iojs, 0.12 and 0.10? Also I'm not sure why do we need to test against so many versions of request.

@nylen nylen mentioned this pull request May 5, 2015
@simov

simov commented May 18, 2015

Copy link
Copy Markdown
Member

@ahmadnassri we finally fixed the tests but it seems that there is a merge conflict, can you resolve that in your branch?

@simov
simov merged commit d356cec into request:master May 20, 2015
@simov

simov commented May 20, 2015

Copy link
Copy Markdown
Member

Merged 👍

@ahmadnassri

Copy link
Copy Markdown
Author

@simov sorry for missing your message! glad you got it fixed & merged.

@simov

simov commented May 20, 2015

Copy link
Copy Markdown
Member

Yep, np, v0.2.0 is published on NPM.

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.

4 participants