Merge common code bases for TdsParserStateObject.cs (3)

[panoskj]

Basically the first two commits of #1844. Plus an optional refactoring. Let me know what you think.

[codecov]

Codecov Report

Attention: 67 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
...oft/Data/SqlClient/TdsParserStateObject.netcore.cs	80.63% <ø> (+2.05%)	⬆️
...osoft/Data/SqlClient/TdsParserStateObject.netfx.cs	83.35% <ø> (+2.28%)	⬆️
...Microsoft/Data/SqlClient/SqlInternalTransaction.cs	72.45% <ø> (ø)
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs	68.59% <0.00%> (-0.16%)	⬇️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs	82.81% <72.38%> (-5.80%)	⬇️

... and 7 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

[panoskj]

I just rebased the branch adding a few commits, in order to follow @David-Engel's suggestion:

On the "breaking it up" suggestion. Troubleshooting the pipeline failures might be easier by breaking it up, too. I'd first try to make changes to the separate TdsParserStateObject.cs files to bring them closer to identical. Ideas for steps in individual PRs:

1. Rename internal variables/properties to all match.

2. Reorder methods/member variables to align the file layout as much as possible, keeping identical code together as much as possible. I might even add some regions and/or comments to help future diffs keep their alignment when common code is removed from each file. (No other changes so that reviewers don't have to scrutinize any differences between the large code blocks.)

3. Move common code into a new common file. Avoid any changes within moved blocks of code.

[Wraith2]

That's still a lot of changes to review with the care we need. I've gone through them all and made notes so I'll put my notes here to help others, function by function.

TryPeekByte - identical in both
TryReadByteArray - functionally identical, netfx adds reliability assert, uses different memory barrier, uses null check for span empty
TryReadByte - functionally identical, netfx adds reliability assert, uses different memory barrier
TryReadChar - functionally identical, netfx uses _bTmp where netcore uses stackalloc, netcore is better
TryReadInt16 - functionally identical, netfx uses _bTmp where netcore uses stackalloc, netcore is better
TryReadInt32 - mostly identical, netfx uses _bTmp where netcore uses stackalloc, netfx uses BinaryConverter where netcore does manual shifting
TryReadInt64 - functionally identical, netfx adds reliability assert
TryReadUInt16 - functionally identical, netfx uses _bTmp where netcore uses stackalloc, netcore is better, no reliability assert?
TryReadUInt32 - functionally identical, netfx uses _bTmp where netcore uses stackalloc, netfx adds reliability assert, netcore is better
TryReadSingle - functionally identical, netfx adds reliability assert
TryReadDouble - functionally identical, netfx adds reliability assert
TryReadString - functionally identical, netfx adds reliability assert
TryReadStringWithEncoding - functionally identical, netfx adds reliability assert
ReadPlpLength - identical in both
TryReadPlpLength - identical in both
ReadPlpBytesChunk - identical in both
TryReadPlpBytes - mostly the same, netcore uses Array.Empty and has support for cached plpbuffer stored in snapshot, netcore is better

It would be good to understand what the reliability assert is supposed to be doing and see if there a way we can make it compile out completely on netcore.

[panoskj]

Good to see you reviewing this @Wraith2, since you wrote #667 and #285. If it helps:

1. TryReadByteArray: Interlocked.MemoryBarrier is a wrapper for Thread.MemoryBarrier according to its documentation (see Remarks section).
2. TryReadByteArray: The null-check that changes to Span.IsEmpty is inside a Debug.Assert condition, so it doesn't affect Release behavior. Moreover, a few lines below it, there is an if statement using Span.IsEmpty rather than the null-check in both netcore and netfx. I didn't go back in commit history to see how it ended up this way yet (will edit this comment if I do).
3. The reliability assert call is compiled out actually, because I annotated the empty stub with [Conditional("NETFRAMEWORK")]. Since the stub's code is only compiled for netcore, the condition will always be false. The reliability assert in netfx is basically a Debug.Assert, checking the that method is running in the correct context (e.g. some reliability functions have been called beforehand).

[Wraith2]

My personal preference would be to use Interlocked.MemoryBarrier everywhere if possible. It fits more closely with the logical intention than using the Thread version to me. Either version would work identically so it's not a huge issue.

For spans since they're structs comparing against null has never made sense to me and I'd prefer to have IsEmpty used everywhere if possible. It was probably me that got it wrong but since you're here you can correct it :)

As long as codegen confirms that the reliability call is entirely compiled out i'm fine with it as you've ported it.

[panoskj]

My personal preference would be to use Interlocked.MemoryBarrier everywhere if possible. It fits more closely with the logical intention than using the Thread version to me. Either version would work identically so it's not a huge issue.

I agree, that's why I chose Interlocked.MemoryBarrier.

For spans since they're structs comparing against null has never made sense to me and I'd prefer to have IsEmpty used everywhere if possible. It was probably me that got it wrong but since you're here you can correct it :)

Comparing against null is a indeed confusing, because of the implicit conversion. You can use Span.Empty instead. However it isn't exactly the same as checking IsEmpty in case of an empty array. In which case, the null-check triggers Debug.Assert while IsEmpty doesn't. But in the end of the day I think it doesn't really matter, since it is just a debug assertion and the code below it handles both null and empty arrays the same way.

As long as codegen confirms that the reliability call is entirely compiled out i'm fine with it as you've ported it.

I think we could add Debug.Assert or throw an exception inside the stub, to verify it is omitted without having to look at the codegen. An exception would also make tests fail. What do you think?

[David-Engel]

What was this? A pull and rebase? A merge on top of your previous commits would have been clear. Please avoid anything that requires a force push on a PR under active review. It makes it really difficult to tell what was changed since the last time someone reviewed. It essentially invalidates all reviewed commits as we can't tell what's changed. This causes a delay in getting your PR reviewed and merged.

Force push clouded previous changes

[panoskj]

@David-Engel My bad, you are right. I will force-push the previously reviewed commits and add a merge commit on top.