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

Migrate from vars to ES6 syntax #46

Merged
merged 2 commits into from
Sep 28, 2020

Conversation

puzpuzpuz
Copy link
Contributor

  • Migrates from vars to ES6 syntax, sot that there are no more vars in the code base
  • Also fixes config typo in check.js

Also fixes config typo in check.js
@puzpuzpuz puzpuzpuz force-pushed the enhancement/improve-code-style branch from 0156f5d to 691f6d9 Compare September 28, 2020 08:20
@mcollina
Copy link
Member

Can you include the results for the benchmark on Node 10, 12 and 14? Those changes usually decrease performance.

@puzpuzpuz
Copy link
Contributor Author

Can you include the results for the benchmark on Node 10, 12 and 14? Those changes usually decrease performance.

Sure. I did 10 runs of the benchSonic*1000 benchmark on this PR and the latest master. Didn't bother with benchmark/compare.R core script, but I can use it if you'd like to have a further analysis on the results.

Summary:
I didn't notice signs of noticeable performance regression. Interestingly, that Node.js 12 is significantly faster than both 10 and 14. To me, it looks like a regression in v12->v14.

This PR:

# Node v10.22.1
benchSonic*1000: 2672.872ms
benchSonic*1000: 2694.200ms
benchSonic*1000: 2692.539ms
benchSonic*1000: 2687.123ms
benchSonic*1000: 2681.746ms
benchSonic*1000: 2723.968ms
benchSonic*1000: 2695.435ms
benchSonic*1000: 2712.366ms
benchSonic*1000: 2694.966ms
benchSonic*1000: 2696.207ms

# Node v12.18.4
benchSonic*1000: 1874.604ms
benchSonic*1000: 1911.222ms
benchSonic*1000: 1933.655ms
benchSonic*1000: 1940.520ms
benchSonic*1000: 1932.524ms
benchSonic*1000: 1894.583ms
benchSonic*1000: 1905.953ms
benchSonic*1000: 1920.437ms
benchSonic*1000: 1929.686ms
benchSonic*1000: 1906.603ms

# Node v14.12.0
benchSonic*1000: 2.400s
benchSonic*1000: 2.416s
benchSonic*1000: 2.396s
benchSonic*1000: 2.395s
benchSonic*1000: 2.382s
benchSonic*1000: 2.377s
benchSonic*1000: 2.397s
benchSonic*1000: 2.394s
benchSonic*1000: 2.372s
benchSonic*1000: 2.394s

Latest master:

# Node v10.22.1
benchSonic*1000: 2687.569ms
benchSonic*1000: 2719.671ms
benchSonic*1000: 2704.618ms
benchSonic*1000: 2711.833ms
benchSonic*1000: 2678.593ms
benchSonic*1000: 2704.299ms
benchSonic*1000: 2693.906ms
benchSonic*1000: 2702.797ms
benchSonic*1000: 2701.660ms
benchSonic*1000: 2703.075ms

# Node v12.18.4
benchSonic*1000: 1909.902ms
benchSonic*1000: 1887.095ms
benchSonic*1000: 1892.965ms
benchSonic*1000: 1850.140ms
benchSonic*1000: 1898.076ms
benchSonic*1000: 1898.124ms
benchSonic*1000: 1931.312ms
benchSonic*1000: 1902.266ms
benchSonic*1000: 1885.535ms
benchSonic*1000: 1856.379ms

# Node v14.12.0
benchSonic*1000: 2.365s
benchSonic*1000: 2.412s
benchSonic*1000: 2.408s
benchSonic*1000: 2.409s
benchSonic*1000: 2.416s
benchSonic*1000: 2.401s
benchSonic*1000: 2.420s
benchSonic*1000: 2.411s
benchSonic*1000: 2.392s
benchSonic*1000: 2.402s

Copy link
Member

@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 63bc3a4 into pinojs:master Sep 28, 2020
@puzpuzpuz puzpuzpuz deleted the enhancement/improve-code-style branch September 29, 2020 06:53
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