Option to use pre-shaped result rows; fixes #3042 #3043
Conversation
… generates pre-shaped result rows
|
More info; #3042 |
koenfaro90
changed the title
|
ohhhh that's super interesting & doesn't seem to introduce any unwanted weird behavior or dynamic class generation or memory leaks or anything. Exciting! I'll run this in CI & see what we can see. 😄 |
|
I managed to run all tests at one point and everything passed, found one
issue with multiple results which had an obvious point to fix.
…On Fri, 11 Aug 2023 at 22:19, Brian C ***@***.***> wrote:
ohhhh that's super interesting & doesn't seem to introduce any unwanted
weird behavior or dynamic class generation or memory leaks or anything.
Exciting! I'll run this in CI & see what we can see. 😄
—
Reply to this email directly, view it on GitHub
<#3043 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADR27XOROD3TAVEZWJ37JPDXU2HUJANCNFSM6AAAAAA3NDNEBU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
All tests pass on all versions of node! FUN! I'm going to pull down your branch and run my horribly crude benchmark against it and current release & i'll report back. |
|
I’d also be interested in how |
You mean for constructing the initial cloneable-object? As that's called only once I would estimate that will be negligible in terms of performance regardless, but might indeed provide protection against a polluted prototype; however, that same risk was present in the current code, so indeed another PR for another day! |
Does your benchmark include manipulating/using the result-rows? That's where the major gain would be, not so much in result construction I think, maybe a little. If you cannot quantify it, then I will write a small benchmark for that next week. |
|
Seems the CI had a hickup, or is something actually failing now? |
|
I wrote a little bench quickly; https://github.com/koenfaro90/node-postgres-bench/tree/master; there seems to be a tiny performance regression SOMEWHERE else, the actual operations seem faster, but somewhere else we seem to loose a little ms - I will investigate on Monday where the regression is caused.
|
|
Nice - I'm assuming on that output above a lower number is better? Should I be paying attention to the It's always kinda cat and mouse game trying to benchmark the driver since postgres may take slightly longer or shorter to do a few queries, node might do a GC pause here or there, network, disk, etc all come into play. I'd recommend trying to run the bench like 10 times & seeing if you steadily see more or less performance from your branch, because in my experience it's not extremely steady numbers....but if actually accessing the row values by name is actually faster, and that speed up is somewhat linearly related to the number of rows returned, this is gonna be yuge! |
|
So, the times you see are all in ms, but the total numbers include setup (generating rows, inserting them), so the actual row usage is waaay faster indeed. Both cloning and simply accessing. But the total time including setup, inserting, querying and executing the specific is slower, perhaps cloning of the row accounts for this, or addFields is called more than once. I will research what accounts for this on Monday.
Cases run in a seperate node instance to prevent any internal optimalization or GC impacting the results. So for now I would hold back on the merge until this is resolved.
…On Sat, 12 Aug 2023 at 17:52, Brian C ***@***.***> wrote:
Nice - I'm assuming on that output above a lower number is better? Should
I be paying attention to the AccessCase and CloneUsingAssignCase etc or
only the diff in numbers between 8.11.2 and pg-performance-pull?
It's always kinda cat and mouse game trying to benchmark the driver since
postgres may take slightly longer or shorter to do a few queries, node
might do a GC pause here or there, network, disk, etc all come into play.
I'd recommend trying to run the bench like 10 times & seeing if you
steadily see more or less performance from your branch, because in my
experience it's not extremely steady numbers....but if actually accessing
the row values by name is actually faster, and that speed up is somewhat
linearly related to the number of rows returned, this is gonna be yuge!
—
Reply to this email directly, view it on GitHub
<#3043 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADR27XIGM5T43G54CIMNNPLXU6RDNANCNFSM6AAAAAA3NDNEBU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Found the issue; needed to ensure that the clone-base record was properly shaped; now the new version is always considerably faster on all fronts (9600ms vs 10000ms for the suite). |
|
amazing work @koenfaro90 ! |
|
I just messaged a related idea to one of my colleagues, focusing on being
able to perform all JS side preparation/serialization while a previous
query is being executed, most gains would be in bulk inserts with a certain
chunk size. But will point him to your issue too.
…On Mon, 14 Aug 2023 at 14:02, Arnaud Benhamdine ***@***.***> wrote:
amazing work @koenfaro90 <https://github.com/koenfaro90> !
by the way, if you are interesting in perf optimizations of this library,
perhaps you would want to work on #2706
<#2706>, it's a POC that I
can't dedicate time, but it would probably improve perfs significantly.
—
Reply to this email directly, view it on GitHub
<#3043 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADR27XJH332GVK2ABR5TSFDXVIHW5ANCNFSM6AAAAAA3NDNEBU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Weirdly tests failed on a few versions of node...rerunning them. Looking forward to merging this!! |
Adds option usePrebuiltEmptyResultObjects on the Query class; this results in generating pre-shaped result row objects instead of dynamically generated ones - massively increasing performance.
I had some difficulties running all tests; also not quite sure about style/backward compatibility demands, so let me know what needs adjusting before LGTM!