Result rows are dynamically shaped; causing massive performance degradations when used

[koenfaro90]

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.

[koenfaro90]

Created a PR: #3043; not sure how to link it.

[charmander]

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.

Yes. Is there a concrete backwards-compatibility issue that prompted an option?

[koenfaro90]

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.

Yes. Is there a concrete backwards-compatibility issue that prompted an option?

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]

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]

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]

Regarding the PR, would you prefer me to modify it to omit the test case and just perform the improvement?

Yep! And thanks for implementing the optimization :)

[koenfaro90]

Added commits to PR removing the test-case and other unneeded mods. Thanks for the quick response!

[brianc]

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.

Yes. Is there a concrete backwards-compatibility issue that prompted an option?

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]

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.

Yes. Is there a concrete backwards-compatibility issue that prompted an option?

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?

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]

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.

Yes. Is there a concrete backwards-compatibility issue that prompted an option?

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?

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!

Most excellent find,.., I wonder what the new benchmark will be compared with postgres.js (claims to be 2x faster then node-postgres)

[koenfaro90]

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.

Yes. Is there a concrete backwards-compatibility issue that prompted an option?

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?

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!

Most excellent find,.., I wonder what the new benchmark will be compared with postgres.js (claims to be 2x faster then node-postgres)

I find that specific benchmark-suite to hardly be real-world-relevant, but as I posted in the PR;

> pg-performance-pull
< pg-performance-pull; 9497ms
> 8.11.2
< 8.11.2; 10749ms
AccessCase { 'pg-performance-pull': 13.71, '8.11.2': 45.76 }
CloneUsingAssignCase { 'pg-performance-pull': 21.21, '8.11.2': 164.82 }
CloneUsingSpreadCase { 'pg-performance-pull': 3.01, '8.11.2': 177.21 }
KeysCase { 'pg-performance-pull': 0.87, '8.11.2': 17.91 }
ValuesCase { 'pg-performance-pull': 10.79, '8.11.2': 77.18 }

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.