Performance | Improve performance of SQLServerDataTable.internalAddRow() function

[cheenamalhotra]

Fixes issue #531 and #539

Rework of PR #538

[codecov-io]

Codecov Report

Merging #990 into dev will decrease coverage by <.01%.
The diff coverage is 80.7%.

@@             Coverage Diff              @@
##                dev     #990      +/-   ##
============================================
- Coverage     53.01%   53.01%   -0.01%     
- Complexity     3158     3161       +3     
============================================
  Files           119      119              
  Lines         28022    28023       +1     
  Branches       4688     4680       -8     
============================================
  Hits          14857    14857              
- Misses        10928    10942      +14     
+ Partials       2237     2224      -13

Impacted Files	Coverage Δ	Complexity Δ
...m/microsoft/sqlserver/jdbc/SQLServerDataTable.java	81.75% <80.7%> (+11%)	42 <0> (+6)	⬆️
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java	43.95% <0%> (-3.3%)	15% <0%> (-1%)
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java	60.87% <0%> (-0.37%)	0% <0%> (ø)
...m/microsoft/sqlserver/jdbc/SQLServerResultSet.java	43.87% <0%> (-0.31%)	324% <0%> (-3%)
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java	68.78% <0%> (+0.11%)	0% <0%> (ø)	⬇️
...n/java/com/microsoft/sqlserver/jdbc/Parameter.java	67.61% <0%> (+0.2%)	64% <0%> (ø)	⬇️
...ncurrentlinkedhashmap/ConcurrentLinkedHashMap.java	39.22% <0%> (+0.43%)	44% <0%> (+1%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b91c7ea...7595479. Read the comment docs.

[cheenamalhotra]

@alexnixon @cogman

I recreated the original PR #538 for improving performance in SQLServerDataTable considering regression scenarios and what we discussed before. I have added tests to cover discussed cases.

Note: I relooked over allowing custom object types when adding row data, and I think that should be supported as it is. The underlying datatype being passed decides the type of the value added to the column and should work as expected.

Let me know if you have any concerns.

[alexnixon]

Hi @cheenamalhotra.

Thanks for revisiting this. Initial thoughts are that having some tests around this is great.

On performance though this PR still leaves lots of cases which go via string serialization. For example using a java.lang.Integer to set a BIGINT field or using a java.lang.Byte to set a BIT field.

The type-testing approach I suggested in this comment on the previous PR handles all these cases and more - do you have an objection to it?

(defaulting to using toString for unknown types as you suggested seems fair enough for backwards compatibility's sake)

[cheenamalhotra]

Hi @alexnixon

I considered that before but it increases code complexity and performs conversions even when not needed (which impacts performance for corresponding types).

With this change, we do give maximum performance for corresponding datatypes, but for values whose datatypes do not correspond to JDBC Type, we support them by converting to String generically... which seems a fair deal to me. It is not something that we recommend for users either way and need not overload implementation for all such possibilities.

[alexnixon]

Could you give me specific example of an unneeded conversion? I'm struggling to see any case in which my suggestion is slower^[1] but can see many important cases which are substantially faster (e.g. the aforementioned using an java.lang.Integer to set a BIGINT field).

I know performance isn't often a big deal but these cases are abysmally slow and given the client is using TVPs it's not unlikely they're shoveling large amount of data through here.

^{[1] unless you count the (BIGINT, Long) case, where I didn't write the Long type-test first, so technically there are a few instructions being wasted. Easy to shuffle that test to the top if you care.}

[cogman]

@alexnixon I believe @cheenamalhotra is referring to all the instance of checks that end up happening.

I believe the complexity tradeoff is fair. In most (all?) cases I'm matching the types up anyways. Even if I didn't do that, I could make the decision to do it at the point of creating the TVP to get better performance with @cheenamalhotra's implementation (I could handle the up/down casting myself)

[cogman]

I don't know what sort of approve authority I have, even if the small changes I suggested aren't implemented I think things look fine.

[cheenamalhotra]

@alexnixon There are many computations going on in that approach, and I think its fair to keep the implementation less complicated inside driver. e.g. working with all Numeric types as Long and then converting them to Short/Integer class objects, etc. also opens up endless possibilities with other types too, whereas it doesn't give much of an advantage vs its need for end user.

And as far as performance is concerned, I agree the corresponding types must be quickly passed through and hence this change is well deserved.

@cogman Thanks, I made quick modifications as per your suggestions. It looks clean now.