Repository navigation
Result rows are dynamically shaped; causing massive performance degradations when used #3042
Description
I have been chasing performance within our applications this week; one of the things I encountered yesterday was that cloning a quite huge amount of data (~500mb), which are actually simple objects returned from node-postgres, blocked the thread for seconds; in itself logical because JSON.parse(JSON.stringify(row)) was being used. I migrated to using the spread operator instead and that should've improved things by a factor of 10-20; it however only worsened the situation.
Upon further examination and reproduction I found the root-cause of the problem; result rows created by node-postgres are dynamically created; one starts with an empty object; and for every result; every field is dynamically created on that object. Turns out, that Javascript engines don't like this because these objects have a non-simple shape. For more info: https://dev.to/fromaline/why-does-dynamically-adding-properties-is-slow-in-javascript-4hm8 / https://youtu.be/5nmpokoRaZI?t=1180
In order to resolve this I have created a patch that, if one passes "usePrebuiltEmptyResultObjects" to the query constructor; will create the row dynamically once; and then use clones of that to create new rows. This result in a 'clean' shape, and massive performance gains when using the rows returned from node-postgres down the line.
I will create a pull-request, including a test-class shortly, which demonstrates that this increases cloning these objects is at least 50x faster; I actually measure more in the range of 150x; but lets leave some room for wiggle.
I would actually prefer to opt to not have this be an option; but standard behaviour; but I would say that is up to the maintainer.
Activity
koenfaro90 commentedon Aug 11, 2023
Created a PR: #3043; not sure how to link it.
charmander commentedon Aug 11, 2023
Yes. Is there a concrete backwards-compatibility issue that prompted an option?
koenfaro90 commentedon Aug 11, 2023
Not as far as I am aware, but this made proving the actual improvement easier. I would not know how to write a relevant test either as this deals with engine internals and cannot be observed to be an improvement without direct comparison.
charmander commentedon Aug 11, 2023
I think the best form of regression check for this is having a benchmark whose performance is tracked across commits. (There aren’t any yet that CI automatically runs and does a nice graph for or anything, though.)
koenfaro90 commentedon Aug 11, 2023
That would most certainly be good, but I lack the time to set that up, there seems to be plenty more clean-up and optimizing to do! If there is any interest in that direction I might be able to free up some resources for that - but I doubt that such a PR would be easily accepted (if at all).
Regarding the PR, would you prefer me to modify it to omit the test case and just perform the improvement?
charmander commentedon Aug 11, 2023
Yep! And thanks for implementing the optimization :)
koenfaro90 commentedon Aug 11, 2023
Added commits to PR removing the test-case and other unneeded mods. Thanks for the quick response!
brianc commentedon Aug 11, 2023
Yeah if there's no backwards compatibility impact (looking at PR I can't see any) I wouldn't even make this optional. Unless this somehow breaks on old node or something this feels like just a straight up massive win for "free" (other than the amazing work & time you put into this @koenfaro90 )
Thoughts?
koenfaro90 commentedon Aug 11, 2023
As discussed in the PR.; made it un-optional, moved all the logic to Result (which is cleaner); also can remove the extra null assignment.
But yes, also happy with this easy win, I think this speeds up basically any property access, but have not explicitly benchmarked that. Should save a lot of trees anyway!
jacobbogers commentedon Aug 13, 2023
Most excellent find,.., I wonder what the new benchmark will be compared with postgres.js (claims to be 2x faster then node-postgres)
koenfaro90 commentedon Aug 14, 2023
I find that specific benchmark-suite to hardly be real-world-relevant, but as I posted in the PR;
AccessCase = looping through all properties of an object retrieved from
CloneUsingAssignCase = Object.assign({}, pgRow);
CloneUsingSpreadCase = { ... pgRow }
KeysCase = Object.keys(pgRow);
ValuesCase = Object.values(pgRow);
Times all in ms; so lower is better; I'm chasing another few smaller wins in value-serializing in utils.js and pg-protocol for which I will create a different issue when done. Used benchmark at https://github.com/koenfaro90/node-postgres-bench/tree/master - will also extend that to include some more realistic/representative cases over the next few days.
Option to use pre-shaped result rows; fixes brianc#3042 (brianc#3043)
perf(pg-native): pre-shaped result rows (#3369)