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.

Sign up for GitHub

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

Jump to bottom

node: improve performance of process.hrtime() #4484

Merged
merged 1 commit into from
Dec 30, 2015
Merged

node: improve performance of process.hrtime() #4484

merged 1 commit into from
Dec 30, 2015

Conversation

evanlucas
Copy link
Contributor

Move argument validation out of C++ and into JS. Improves performance
by about 15-20%.

Benchmark comparisons between current master and this branch are below:

$ node benchmark/compare.js ./node ./node_before  -- misc bench-hrtime
running ./node
misc/bench-hrtime.js
running ./node_before
misc/bench-hrtime.js

misc/bench-hrtime.js n=1000000: ./node: 13037000 ./node_before: 10677000 . 22.11%


$ node benchmark/compare.js ./node ./node_before  -- misc bench-hrtime
running ./node
misc/bench-hrtime.js
running ./node_before
misc/bench-hrtime.js

misc/bench-hrtime.js n=1000000: ./node: 12954000 ./node_before: 10642000 . 21.73%


$ node benchmark/compare.js ./node ./node_before  -- misc bench-hrtime
running ./node
misc/bench-hrtime.js
running ./node_before
misc/bench-hrtime.js

misc/bench-hrtime.js n=1000000: ./node: 13322000 ./node_before: 10770000 . 23.69%

R= @trevnorris

@evanlucas
Copy link
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/1113/

@trevnorris trevnorris added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 30, 2015
@trevnorris
Copy link
Contributor

good catch. LGTM.

@jasnell
Copy link
Member

jasnell commented Dec 30, 2015

CI failures are unrelated. LGTM

@evanlucas
node: improve performance of process.hrtime()
78fd435 
Move argument validation out of C++ and into JS. Improves performance
by about 15-20%.

PR-URL: nodejs#4484
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@evanlucas evanlucas closed this Dec 30, 2015
@evanlucas evanlucas deleted the hrtime branch December 30, 2015 22:55
@evanlucas evanlucas merged commit 78fd435 into nodejs:master Dec 30, 2015
@evanlucas
Copy link
Contributor Author

Landed in 78fd435. Thanks!

Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
@evanlucas @Fishrock123
node: improve performance of process.hrtime()
921fb54 
Move argument validation out of C++ and into JS. Improves performance
by about 15-20%.

PR-URL: nodejs#4484
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
@Fishrock123
2016-01-06, Version 5.4.0 (Stable)
af591d4 
* http:
  - A new status code was added: 451 - "Unavailable For Legal Reasons" (Max Barinov) nodejs#4377
  - Idle sockets that have been kept alive now handle errors (José F. Romaniello) nodejs#4482
* This release also includes several minor performance improvements:
  - assert: deepEqual is now speedier when comparing TypedArrays (Claudio Rodriguez) nodejs#4330
  - lib: Use arrow functions instead of bind where possible (Minwoo Jung) nodejs#3622
  - node: Improved accessor perf of process.env (Trevor Norris) nodejs#3780
  - node: Improved performance of process.hrtime() (Trevor Norris) nodejs#3780, (Evan Lucas) nodejs#4484
  - node: Improved GetActiveHandles performance (Trevor Norris) nodejs#3780
  - util: Use faster iteration in util.format() (Jackson Tian) nodejs#3964

PR-URL: nodejs#4547
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Jan 11, 2016
@Fishrock123
2016-01-06, Version 5.4.0 (Stable)
5f0abd6 
* http:
  - A new status code was added: 451 - "Unavailable For Legal Reasons"
(Max Barinov) nodejs#4377
  - Idle sockets that have been kept alive now handle errors (José F.
Romaniello) nodejs#4482
* This release also includes several minor performance improvements:
  - assert: deepEqual is now speedier when comparing TypedArrays
(Claudio Rodriguez) nodejs#4330
  - lib: Use arrow functions instead of bind where possible (Minwoo
Jung) nodejs#3622
  - node: Improved accessor perf of process.env (Trevor Norris)
nodejs#3780
  - node: Improved performance of process.hrtime() (Trevor Norris)
nodejs#3780, (Evan Lucas)
nodejs#4484
  - node: Improved GetActiveHandles performance (Trevor Norris)
nodejs#3780
  - util: Use faster iteration in util.format() (Jackson Tian)
nodejs#3964

Refs: nodejs#4547
PR-URL: nodejs#4623
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins
Copy link
Contributor

@evanlucas this is not merging into v4.x-staging without conflicts. Would you be able to take a look and perhaps send a new PR against that branch?

@evanlucas
Copy link
Contributor Author

Sure I'll try do get that done tonight.

@evanlucas
Copy link
Contributor Author

@thealphanerd this one depends on 36e8a2c, which doesn't appear to have landed in v4.x-staging, so we should probably avoid backporting this one unless that one gets backported as well?

@MylesBorins
Copy link
Contributor

it looks like 36e8a2c is part of a series of six commits that were not landing cleanly. Work done by @trevnorris

Do you think that it is worth the effort to attempt to back port all of this?

@jasnell
Copy link
Member

jasnell commented Jan 14, 2016

I would say let's hold off for now.
On Jan 14, 2016 4:21 AM, "Myles Borins" [email protected] wrote:

it looks like 36e8a2c
36e8a2c
is part of a series of six commits that were not landing cleanly. Work done
by @trevnorris https://github.com/trevnorris

Do you think that it is worth the effort to attempt to back port all of
this?

—
Reply to this email directly or view it on GitHub
#4484 (comment).

@MylesBorins
Copy link
Contributor

As #3780 is now dont-land-on-v4.x I'm going to mark this one similarly

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@evanlucas
node: improve performance of process.hrtime()
027d385 
Move argument validation out of C++ and into JS. Improves performance
by about 15-20%.

PR-URL: nodejs#4484
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@Fishrock123
2016-01-06, Version 5.4.0 (Stable)
3991aa8 
* http:
  - A new status code was added: 451 - "Unavailable For Legal Reasons"
(Max Barinov) nodejs#4377
  - Idle sockets that have been kept alive now handle errors (José F.
Romaniello) nodejs#4482
* This release also includes several minor performance improvements:
  - assert: deepEqual is now speedier when comparing TypedArrays
(Claudio Rodriguez) nodejs#4330
  - lib: Use arrow functions instead of bind where possible (Minwoo
Jung) nodejs#3622
  - node: Improved accessor perf of process.env (Trevor Norris)
nodejs#3780
  - node: Improved performance of process.hrtime() (Trevor Norris)
nodejs#3780, (Evan Lucas)
nodejs#4484
  - node: Improved GetActiveHandles performance (Trevor Norris)
nodejs#3780
  - util: Use faster iteration in util.format() (Jackson Tian)
nodejs#3964

Refs: nodejs#4547
PR-URL: nodejs#4623
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@evanlucas @trevnorris @jasnell @MylesBorins