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

Possible performance tuning. #1

Open
Raynos opened this issue Jul 18, 2016 · 2 comments
Open

Possible performance tuning. #1

Raynos opened this issue Jul 18, 2016 · 2 comments

Comments

@Raynos
Copy link

Raynos commented Jul 18, 2016

I replaced two lines in my code base from:

var copy = JSON.parse(JSON.stringify(o))

to

var copy = universalCopy(o)

And it increased the runtime of test suite from 4 seconds to 5 seconds.

My code base is a type checker, the main place i use universalCopy() is to copy the type definition files AST, which is a large data structure.

JSON.parse(JSON.stringify()):

$ time node test/index.js | grep -v '^ok' | grep -v '^# '
pid: 28829
TAP version 13
1..12275

real    0m16.841s
user    0m16.800s
sys 0m0.256s

universal-copy:

$ time node test/index.js | grep -v '^ok' | grep -v '^# '
pid: 28711
TAP version 13
1..12275

real    0m31.591s
user    0m31.464s
sys 0m0.420s

I also created two flamegraphs:

Image of unversal-copy

  • Looking at the wallclock time, in this stress test the performance was 50%
  • in my actual test-suite the performance dropped by 20%.
  • Looking at the flamegraph in detail, JSON.stringify/JSON.parse is 800% faster in isolation.

Also, from these flames, it looks like the other huge CPU hog is my naively implemented language parser using the parsimmon framework :)

It would be cool if benchmarks can be added and this library can be performance tuned.

Once I start performance tuning my application, I can optimize the copy() function and contribute the code back (it might make simplification trade-offs for performance... and no longer be "universal")

@Raynos
Copy link
Author

Raynos commented Jul 18, 2016

Btw:

If node-flame doesn't work, then you could also play with 0x ( https://github.com/davidmarkclements/0x ) which I've never personally used.

@nrn
Copy link
Owner

nrn commented Jul 18, 2016

Thanks for all the details. I'll dig into it when I can.

Off the top of my head, the FakeMap that gets used if you don't have native maps available probably isn't performant, and moveProps is costly in the name of correctness.

Trying gutting moveProps to just do a for in loop and not worry about symbols/property descriptor nonsense is the first thing to look at to sacrifice completeness for perf.

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

2 participants