dont use dynamic functions to parse rows, closes #1417 #1603
Conversation
| @@ -65,29 +64,24 @@ Result.prototype._parseRowAsArray = function (rowData) { | |||
| return row | |||
| } | |||
|
|
|||
| // rowData is an array of text or binary values | |||
| // this turns the row into a JavaScript object | |||
| Result.prototype.parseRow = function (rowData) { | |||
There was a problem hiding this comment.
This is a simple reduce but I kept the same style as the array parser for consistency.
|
Is there a reason for this change? Last time we checked (~6 months ago) the eval provides around at 30% speed up over setting fields like you're doing. Is the current code causing you problems? |
|
Ah i see now in the referenced issue, sorry missed that in the PR comments here. |
|
@brianc Would love to know how you got those #s so I can reproduce them and refine this to match, or think of an alternative solution so we don't lose any performance. Ultimately though, if the dynamic function stuff is an inherent memory issue I think not crashing from out of memory errors is more important than 30% faster. |
|
One thing is the dynamic function stuff has been in the code for ~5 years. I am slightly circumspect of it causing issues that have only been discovered now given how widely and heavily used this module is across the node ecosystem. As far as the benchmarking goes - just run a query that returns 500 rows w/ the npm published version of pg. Run it 10,000 times and take the median execution time. Now do the same w/ this patch - compare. When I get some time I'll take a look at the script provided in the issue & see if there's anything to track down. Also interested in @charmander's thoughts. |
|
@brianc Yeah, that also strikes me as strange. We only started having issues with actual OOM crashes when our writes hit ~10k RPM - I would assume somebody out there has a workload of > ~10K queries per minute on this library, but maybe not. |
|
@contra interestingly I had the exact same thing, when using pg "reasonably" I had no issue, until one of my workers had to insert thousands of records per minute. Really curious why the rate at which you send queries would impact memory usage! |
|
I used this fix and here are the results: before after (the above is 4 pods in one cluster, so it's the total for all 4) |
|
I'm a colleague of @knownasilya and can vouch that his app isn't doing anything obviously stupid or weird, just a bunch of select statements |
| for (var i = 0; i < fieldDescriptions.length; i++) { | ||
| var desc = fieldDescriptions[i] | ||
| this.fields.push(desc) | ||
| var parser = this._getTypeParser(desc.dataTypeID, desc.format || 'text') | ||
| this._parsers.push(parser) | ||
| // this is some craziness to compile the row result parsing | ||
| // results in ~60% speedup on large query result sets | ||
| ctorBody += inlineParser(desc.name, i) |
There was a problem hiding this comment.
if you guys are dead set against this pull, then at least one improvement that could be definitely made to pg is to make ctorBody an array, have this line push the inline parser string into the array and then do an array.join at the end, currently here we are making 2 strings for each field of the results
|
I'm not dead set against it. One thing you can do in the mean time before I merge this is use |
|
+1 on this merge. We are facing this and currently looking at options to resolve if the module is not updated. Fixing this leak would greatly help us as well. |
|
++1 We are also having issues with the leak on a high traffic endpoint. Would love to to see this merged 🚀 |
|
Going to merge this into the |
|
will there be a v6 release with this change? we are unable to upgrade to v7 at this time unfortunately. |
|
We backported this fix to v5 because we ran into other issues. Will try to mention those once I can find the error we had 👍 |
Test pass, next step is benchmarks - I could use a hand with those if you have time, but I think we should be able to optimize this further to beat the dynamic function performance.