Modify SqlStreamingXml XmlWriter to internally use direct XmlReader parsing

[jimhblythe]

Modify SqlStreamingXml XmlWriter to internally use a MemoryStream instead of a StringBuilder.

Note: UTF8Encoding(false) addition in s_writerSettings is consistent with prior default used within StringWriter/StringBuilder

Issues

Fixes #1877 to be O(n)

Testing

Added 2 new Manual tests to ensure linear behavior for single large node, and secondary validation for multiple nodes

[jimhblythe]

@dotnet-policy-service agree

[paulmedynski]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[paulmedynski]

Looks great - thanks for this optimization and expanded test coverage! Just one question.

[paulmedynski]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[codecov]

Codecov Report

❌ Patch coverage is 97.57576% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.82%. Comparing base (a68e00f) to head (9e62ba5).
⚠️ Report is 65 commits behind head on main.

Files with missing lines	Patch %	Lines
...qlClient/src/Microsoft/Data/SqlClient/SqlStream.cs	97.57%	4 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (a68e00f) and HEAD (9e62ba5). Click for more details.

HEAD has 6 uploads less than BASE

Flag	BASE (a68e00f)	HEAD (9e62ba5)
netfx	2	0
netcore	2	0
addons	2	0

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3974      +/-   ##
==========================================
- Coverage   75.22%   65.82%   -9.40%     
==========================================
  Files         266      275       +9     
  Lines       42932    65896   +22964     
==========================================
+ Hits        32294    43379   +11085     
- Misses      10638    22517   +11879     

Flag	Coverage Δ
PR-SqlClient-Project	65.82% <97.57%> (?)
addons	?
netcore	?
netfx	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mdaigle]

@jimhblythe can you take a look at the tests in #4005 and #4008? They seem to suggest that this introduces a regression for non-ascii characters. You can check the pipeline results to see the test outcomes. I'd like to see some tests like that included in this PR as well, please 🙏

[jimhblythe]

@jimhblythe can you take a look at the tests in #4005 and #4008? They seem to suggest that this introduces a regression for non-ascii characters. You can check the pipeline results to see the test outcomes. I'd like to see some tests like that included in this PR as well, please 🙏

I will additionally including a fourth test to verify surrogate pairs:

        [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))]
        public static void GetChars_SurrogatePairContent()
        {
            SqlConnection connection = new(DataTestUtility.TCPConnectionString);
            // Surrogate Pair characters: 4 bytes each in UTF-8
            string xml = "<data>\U0001F600\U0001F525\U0001F680</data>";

Actual: "<data>😀🔥🚀</data>"

The test shows these worked with prior code.

[jimhblythe]

I had to use an alternate as MemoryStream was not the right choice. I actually optimized further with bypass of XmlWriter which used an additional buffer; more memory spared with latest commit.

Significantly more tests added :

Is exact replication of prior logic desired or more consistent with how SSMS would show an XML column?
Prior logic would occasionally convert < back to < - e.g. within CData
Prior logic would also expand \n to \r\n

Coverage complete except for null reference bullet-proofing that cannot be replicated through SqlReader which ensures values are provide:

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[paulmedynski]

Hey @jimhblythe - Nothing further at the moment. We're just tied up with other tasks. I have tentatively put this into the next preview release, but it looks like a substantial change to review. It may get bumped to preview 2.

[benrr101]

Thanks for the submission - this would be a great performance improvement to take!

Definitely want to see the tests rewritten as unit tests, and I'd really like to avoid rewriting xml entities escaping code. But feel free to push back if I misunderstood something - just be prepared to document why it's being done that way :)

[priyankatiwari08]

Thanks for the perf work on #1877 — algorithmic fix seems to be in the right direction. Agree with @benrr101's asks; nothing material to add on top of his review, just two small nits below.

[jimhblythe]

@benrr101 @priyankatiwari08 Thanks for the reviews and feedback.

I did not mark any conversations as resolved unless I addressed them with a code modification. For the remaining open items, could you please review and either reply with any additional considerations or resolve if you feel the current explanation is sufficient?

[benrr101]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[benrr101]

I think the explanations make sense, and the changes look good. I'm ok to take these. Thank you again for you contribution!

[paulmedynski]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[benrr101]

@jimhblythe I took the liberty to fix the issue with the manual test project file that was causing the pipeline failures. Hopefully we'll get it moving

[benrr101]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).