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

Drop Usage of Object::Set() #3780

Closed
wants to merge 6 commits into from
Closed

Conversation

trevnorris
Copy link
Contributor

Several performance improvements by removing usage of Object::Set().

R=@bnoordhuis

@mscdex
Copy link
Contributor

mscdex commented Nov 11, 2015

Can you share the benchmark results you're seeing before and after these optimizations?

@r-52 r-52 added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 11, 2015
@trevnorris
Copy link
Contributor Author

@mscdex Sure. Here's a round from my laptop:

               before        after
env:          17.8 us/op    6.4 us/op
readdir:      47.5 us/op   36.7 us/op
readdirSync:  36.8 us/op   25.5 us/op
parser:       4687 ns/op   2731 ns/op
hrtime:        688 ns/op     88 ns/op

}

t = process.hrtime(t);
console.log(((t[0] * 1e9 + t[1]) / ITER).toFixed(1) + ' ns/op');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you be using common.createBenchmark() in these benchmarks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like common.createBenchmark() is a fail for benchmarking operations that finish in microseconds or less because at that point we're hitting the lower limit of how many CPU instructions it may take to perform an operation. Displaying the amount of time per operation is more insightful into what's actually going on, IMHO. But to stay consistent I can move the benchmarks over to common. That won't ruffle my feathers.

@targos
Copy link
Member

targos commented Nov 11, 2015

How did you choose the value of 8 for all the argc/name_c ?

@@ -178,12 +178,29 @@
}

startup.setupProcessObject = function() {
process._setupProcessObject(setPropByIndex);
const _hrtime = process.hrtime;
var hrValues = new Uint32Array(4);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Future optimization: instead of allocating many small but long-lived typed arrays in many places, allocate a single one early on and use fixed offsets in places like this.

@trevnorris
Copy link
Contributor Author

@targos In a previous PR (#3375 (diff)) @bnoordhuis requested the value stay a power of 2. Did some benchmarking and found that the point of diminishing returns was 8.

@trevnorris trevnorris force-pushed the drop-all-set branch 3 times, most recently from d0a0faf to 7cbc982 Compare November 11, 2015 23:41
@@ -214,6 +214,10 @@ static void After(uv_fs_t *req) {
{
int r;
Local<Array> names = Array::New(env->isolate(), 0);
Local<Function> fn = env->push_values_to_array_function();
static const size_t name_argc = 8;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be moved to a common header file or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Done.

@trevnorris trevnorris force-pushed the drop-all-set branch 4 times, most recently from 2111652 to 108021e Compare November 13, 2015 00:03
@trevnorris
Copy link
Contributor Author

@bnoordhuis code comments addressed. let me know your preference for the benchmarks, and if you'd like any more information on readdir().

@trevnorris
Copy link
Contributor Author

@bnoordhuis Anything else you'd like that I take care of?

@ronkorving
Copy link
Contributor

What magic is this? :)
Alright, clearly the performance improvement is worth it. But is there anything that can be done to explain what the heck is going on here? For us mere mortals, you know... Perhaps through a clever macro, or otherwise.

Small sidenote, but as my name came up re. onboarding, it would be great if someone somewhere could point contributors to some "getting started with V8 in Node" wiki/md-collection where tricks like this could be documented and maintained.

if (w->persistent().IsEmpty())
continue;
argv[idx] = w->object();
if (++idx >= NODE_PUSH_VAL_TO_ARRAY_MAX) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use ARRAY_SIZE(argv) here and elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took care of all these locations. Note in node_http_parser.cc I had to do ARRAY_SIZE() / 2.

@trevnorris trevnorris force-pushed the drop-all-set branch 2 times, most recently from e3f52ef to 5bcd693 Compare November 24, 2015 19:13
@trevnorris
Copy link
Contributor Author

@ronkorving You mean, give an explanation somewhere why this technique is faster, or make it simpler to deal w/ my using a macro or similar so others down the road know how to use this same technique? good idea documenting these things. can think of at least 2 i'm responsible for that would be confusing.

Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
The initial implementation of setPropByIndex() set the value of an Array
by index during development. Though the final form of the function
simply pushes passed values to an array as passed by arguments. Thus the
functions have been renamed to pushValueToArray() and
push_values_to_array_function() respectively.

Also add define for maximum number of arguments should be used before
hitting the limit of performance increase.

Fixes: 494227b "node: improve GetActiveRequests performance"
PR-URL: nodejs#3780
Reviewed-By: Fedor Indutny <[email protected]>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
For performance add headers to the headers Array by pushing them on from
JS. Benchmark added to demonstrate this case.

PR-URL: nodejs#3780
Reviewed-By: Fedor Indutny <[email protected]>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
Improve performance by pushing directory entries to returned array in
batches of 8 using pushValueToArray() in JS. Add benchmarks to
demonstrate this improvement.

PR-URL: nodejs#3780
Reviewed-By: Fedor Indutny <[email protected]>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
Improve performance of process._getActiveHandles by sending handles in
batches to JS to be set on the passed Array. Add test to check proper
active handles are returned.

Alter implementation of GetActiveRequests to match GetActiveHandles'
implementation.

PR-URL: nodejs#3780
Reviewed-By: Fedor Indutny <[email protected]>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
process.hrtime() was performing too many operations in C++ that could be
done faster in JS. Move those operations over by creating a length 4
Uint32Array and perform bitwise operations on the seconds so that it was
unnecessary for the native API to do any object creation or set any
fields.

This has improved performance from ~350 ns/op to ~65 ns/op. Light
benchmark included to demonstrate the performance change.

PR-URL: nodejs#3780
Reviewed-By: Fedor Indutny <[email protected]>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
Set process.env array entries in JS.

PR-URL: nodejs#3780
Reviewed-By: Fedor Indutny <[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]>
@rvagg
Copy link
Member

rvagg commented Jan 15, 2016

labelling as dont-land-on-v4.x, this is too heavy-handed for LTS I think although it is going to increase the difficulty of backporting code around the touched parts.

@jasnell
Copy link
Member

jasnell commented Jan 15, 2016

@rvagg @thealphanerd ... +1 .. I appreciate the additional review

@trevnorris trevnorris deleted the drop-all-set branch March 28, 2016 22:28
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
The initial implementation of setPropByIndex() set the value of an Array
by index during development. Though the final form of the function
simply pushes passed values to an array as passed by arguments. Thus the
functions have been renamed to pushValueToArray() and
push_values_to_array_function() respectively.

Also add define for maximum number of arguments should be used before
hitting the limit of performance increase.

Fixes: 494227b "node: improve GetActiveRequests performance"
PR-URL: nodejs#3780
Reviewed-By: Fedor Indutny <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
For performance add headers to the headers Array by pushing them on from
JS. Benchmark added to demonstrate this case.

PR-URL: nodejs#3780
Reviewed-By: Fedor Indutny <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Improve performance by pushing directory entries to returned array in
batches of 8 using pushValueToArray() in JS. Add benchmarks to
demonstrate this improvement.

PR-URL: nodejs#3780
Reviewed-By: Fedor Indutny <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Improve performance of process._getActiveHandles by sending handles in
batches to JS to be set on the passed Array. Add test to check proper
active handles are returned.

Alter implementation of GetActiveRequests to match GetActiveHandles'
implementation.

PR-URL: nodejs#3780
Reviewed-By: Fedor Indutny <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
process.hrtime() was performing too many operations in C++ that could be
done faster in JS. Move those operations over by creating a length 4
Uint32Array and perform bitwise operations on the seconds so that it was
unnecessary for the native API to do any object creation or set any
fields.

This has improved performance from ~350 ns/op to ~65 ns/op. Light
benchmark included to demonstrate the performance change.

PR-URL: nodejs#3780
Reviewed-By: Fedor Indutny <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Set process.env array entries in JS.

PR-URL: nodejs#3780
Reviewed-By: Fedor Indutny <[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 free to 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.