Skip to content

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
ContributorAuthor

@trevnorristrevnorris 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

CI failures are unrelated. LGTM

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]>
@evanlucasevanlucas deleted the hrtime branch December 30, 2015 22:55
@evanlucasevanlucas merged commit 78fd435 into nodejs:masterDec 30, 2015
@evanlucas
Copy link
ContributorAuthor

Landed in 78fd435. Thanks!

Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
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
* 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
* 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
ContributorAuthor

Sure I'll try do get that done tonight.

@evanlucas
Copy link
ContributorAuthor

@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

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 @trevnorrishttps://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
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
* 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@evanlucas@trevnorris@jasnell@MylesBorins