perf(pg): avoid useless spread

[cesco69]

Just a micro-optimization for avoid useless spread operator on object

[brianc]

👍 🙇

[charmander]

The spread was intentional. Did you benchmark this?

[cesco69]

The spread was intentional. Did you benchmark this?

Yep! but the difference is negligible. In multiple run, sometimes wins the spread version, but I think is just because we talking about a very little difference. Honestly I have read the comment but not fully undestand why the spread should be improve the performance, it create a new object!

Maybe @koenfaro90 can help us

@charmander do you have noticed some slow down in the latest version?

[koenfaro90]

@cesco69 @charmander @brianc

Constructing using spread itself is not any faster or slower. But USING objects generated with a 'fixed' shape is MUCH MUCH faster than using objects which are dynamically shaped. So if you start with an object with 0 properties and continually add properties the shape of that object will be complex - versus assigning all of them at initialization which leads to a simple shape.

Some more background at

https://v8.dev/blog/fast-properties
https://mathiasbynens.be/notes/shapes-ics

The patch I made ensured that objects have a 'simple' shape. This results in significant (IIRC 10-100x) speed-up when doing something like JSON.stringify on the returned objects - considering the objects contain a significant amount of columns.

[cesco69]

@cesco69 @charmander @brianc

Constructing using spread itself is not any faster or slower. But USING objects generated with a 'fixed' shape is MUCH MUCH faster than using objects which are dynamically shaped. So if you start with an object with 0 properties and continually add properties the shape of that object will be complex - versus assigning all of them at initialization which leads to a simple shape.

Some more background at

https://v8.dev/blog/fast-properties https://mathiasbynens.be/notes/shapes-ics

The patch I made ensured that objects have a 'simple' shape. This results in significant (IIRC 10-100x) speed-up when doing something like JSON.stringify on the returned objects - considering the objects contain a significant amount of columns.

Thanks for the detailed response! In your opinion my changes https://github.com/brianc/node-postgres/pull/3379/files can be a problem?

[koenfaro90]

@cesco69 @charmander @brianc

Yes; this removes the performance fix as now you are using a dynamically shaped object to clone from, resulting in a major performance regression for any application using this library. So in my opinion this should be reverted before the next release. Perhaps we should add some tests regarding the performance of using the objects resulting from PG?

I have a repo running some tests:

https://github.com/koenfaro90/node-postgres-bench/tree/master

[cesco69]

Yes; this removes the performance fix as now you are using a dynamically shaped object to clone from, resulting in a major performance regression for any application using this library. So in my opinion this should be reverted before the next release. Perhaps we should add some tests regarding the performance of using the objects resulting from PG? I don't have mine handy anymore right now.

ok, I have understood, thanks. Sorry for my mistake!

With this PR #3382 revert both in pg and pg-native

[jorrit]

Perhaps add some comments to explain the reason why these spreads are necessary.

[cesco69]

I have a repo running some tests:

https://github.com/koenfaro90/node-postgres-bench/tree/master

@koenfaro90 version 8.13.2 (the version with my PR) has introduced slowness compared to 8.13.1

< 8.13.1; 30995ms
< 8.13.2; 31712ms
< 8.13.3; 30551ms

Side note, this project is gold https://github.com/koenfaro90/node-postgres-bench/tree/master and should be part of this repo!