-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
perf: pre allocate array instead of push item #3250
Conversation
Thanks for bringing this up. Do you have any benchmark results? |
The PR is 20% faster. Tested only on node 20. |
20% faster on what benchmark? |
I have made a benchmark of single method, but moking the db/query logic, just for test if Preallocating array has a performance benefit, eg consumeFields |
pre allocate array is generally faster compared to other methods, push is the slower |
@charmander after fixed dev container with #3251 I have also run the pg-native bench https://github.com/brianc/node-postgres/blob/master/packages/pg-native/bench/index.js TL;TR The benchmark shows no improvement. But looking at the benchmark code, this doesn't appear to be very accurate PR
MASTER
|
Hi @charmander also on the vanilla "result object" the array length is pre allocate, why not on native? https://github.com/brianc/node-postgres/blob/master/packages/pg/lib/result.js#L51 This PR just align the behaviour. Side note: Side note: preallocate and fill is better, eg.: const size = 1_000;
const arr = new Array(size).fill(null);
for(let i = 0; i<size; i++){
arr[i] = {foo: "foo"};
} see benchmark |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey this looks good to me! Definitely don't see the harm in this at all, and if it speeds things up, that's a win!
just a little performance improvement. Preallocating array instead of push item