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

perf: pre allocate array instead of push item #3250

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

nigrosimone
Copy link
Contributor

just a little performance improvement. Preallocating array instead of push item

@charmander
Copy link
Collaborator

Thanks for bringing this up. Do you have any benchmark results?

@nigrosimone
Copy link
Contributor Author

Thanks for bringing this up. Do you have any benchmark results?

The PR is 20% faster. Tested only on node 20.

@charmander
Copy link
Collaborator

20% faster on what benchmark?

@nigrosimone
Copy link
Contributor Author

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

@nigrosimone
Copy link
Contributor Author

pre allocate array is generally faster compared to other methods, push is the slower
benchmark

@nigrosimone
Copy link
Contributor Author

nigrosimone commented Jun 8, 2024

@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

node index.js

pure done 49
native done 36
pure done 70
native done 36
pure done 51
native done 43
pure done 25
native done 38
pure done 20
native done 27
pure done 35
native done 29
pure done 34
native done 54
pure done 33
native done 58
pure done 55
native done 24
pure done 31
native done 38
pure done 25
native done 25
pure done 19
native done 29
pure done 32
native done 36
pure done 54
native done 46

MASTER

node index.js

pure done 73
native done 60
pure done 29
native done 44
pure done 15
native done 33
pure done 44
native done 29
pure done 15
native done 47
pure done 15
native done 50
pure done 15
native done 30
pure done 15
native done 37
pure done 16
native done 17
pure done 23
native done 36
pure done 43
native done 35
pure done 17
native done 19
pure done 17
native done 39
pure done 15
native done 45
pure done 15
native done 31
pure done 39
native done 27
pure done 16
native done 39

@cesco69
Copy link

cesco69 commented Oct 29, 2024

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

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.

3 participants