Improving buildParamTypeDefinitions performance

[cogman]

It was noticed in our applications that the growing of arrays and string builders were a major source of memory pressure. Precomputing the size of both and removing an unneeded string allocation from trim significantly reduces the amount of allocation buildParamTypeDefinitions needs.

[cogman]

@microsoft-github-policy-service agree

[Jeffery-Wasty]

Thank you for this PR. We'll look over it, test it out, and get back to you as soon as we can.

[Jeffery-Wasty]

We're not seeing this 'memory pressure' you talk about. Would you be able to provide screenshots showing memory impact before and after this fix? This will help us in prioritizing this PR.

[cogman]

@Jeffery-Wasty Yup.

Let me recreate the scenario to show the reduction. In our case, we are dealing with batch statements against very wide tables so that's what I'll put together to show it.

[cogman]

Flight recording + java used to generate results

See the attached. It's not a perfect win but it does cut back on the number of garbage collections (reducing memory pressure) while doing batch inserts. As an added bonus, because it isn't copying as much data around it also reduces the runtime of that method.

Both flight recordings are for 1 minute after the JVM had ran for a bit with the attached code.

[Jeffery-Wasty]

@cogman, this PR is being moved to our backlog for now, and will be assessed again as soon we can. We're doing this for two reasons:

• The code makes changes to very delicate parts of the code and extensive testing is required before incorporating changes.
• The memory difference noted doesn't appear to be a large one. From the screenshots you posted, you are going from ~5% usage to ~1%. While this is an improvement, and we're always looking to improve the driver, it's not enough to make this a high priority item at this moment.

[cogman]

@Jeffery-Wasty No worries.

2 points though, the important thing about my screenshots wasn't necessarily the CPU improvements (though, those are there) but rather the amount of memory allocated. The number of GCs for the same runtime is roughly half. As I mentioned earlier, this PR is about reducing memory pressure (number of GCs) above all else. For our larger applications this is important to us. More garbage collecting == lower throughput.

Second point is this code, because of it's location, is very heavily tested. When writing this, I noticed the method was invoked in test over 400k times. That's the primary reason I didn't add new tests, the code is thoroughly covered.

Again, no rush, but just thought those are relevant points.

[Jeffery-Wasty]

closes #1678

[Jeffery-Wasty]

Hi @cogman,

We've removed this from our backlog as we're now able to spend time looking into it again. We'll update you on the status as we continue.

[David-Engel]

LGTM

[tkyc]

I found one issue with the PR when I was doing some testing on our internal pipelines. At the moment I'm still trying to debug the issue and identify the cause, but I've attached a file with the failing test and DDL used in case you want to take a look as well (the test in the file will need to be ran on an AE connection). So far from what I've learned, the issue happens because the renewed build type definition isn't using a precision of 3 for the type ie. it's setting the renewed build type definition as datetime rather than datetime2(3) which is the reason for the failing test.

PR1938-debug.txt

EDIT:

This is likely the fix. Line 470 will need to happen before line 442.

            params[i].renewDefinition = renewDefinition;
            String typeDefinition = param.getTypeDefinition(connection, resultsReader());

[cogman]

@tkyc I've made that change. Let me know if there's further changes needed.

[tkyc]

@cogman Everything looks good so far, I'll merge after my tests are done running.

[tkyc]

Just a heads up, next preview release will be early June and the stable release will be on end of July.