- Notifications
You must be signed in to change notification settings - Fork311
NativeAOT | Remove DataSet.ReadXml, correct type name#3369
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This removes a reference to the built-in XML deserialization, which is slower and isn't trim-safe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A few questions to help with my understanding
Uh oh!
There was an error while loading.Please reload this page.
src/Microsoft.Data.SqlClient/src/Resources/Microsoft.Data.SqlClient.SqlMetaData.xmlShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbMetaDataFactory.csShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Asking for some clarification on why we're reading this XML at runtime.
src/Microsoft.Data.SqlClient/src/Resources/Microsoft.Data.SqlClient.SqlMetaData.xmlShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
codecovbot commentedMay 22, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@## main #3369 +/- ##==========================================- Coverage 65.12% 62.22% -2.90%========================================== Files 300 294 -6 Lines 65379 65183 -196 ==========================================- Hits 42578 40563 -2015- Misses 22801 24620 +1819
Flags with carried forward coverage won't be shown.Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a1bfea8
intodotnet:mainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Description
This PR's diff is much smaller than it looks! There's an embedded XML resource, and adjusting this meant that the indentation levels changed. When that's accounted for, the PR becomes much smaller - around 300 lines, rather than 7800.
This PR relates to#1942 and#1261. At present, there are two identical copies of the
Microsoft.Data.SqlClient.SqlMetaData.xml
embedded resource in the netfx and the netcore projects. This PR merges these with a single file. More importantly though, this embedded resource is used only withinDbMetaDataFactory
; it's deserialized usingDataSet.ReadXml
, where it is eventually used inSqlConnection.GetSchema
.DataSet.ReadXml
isn't trim-safe - it uses XML serialization in the background. I've replaced this with a simple parsing routine which runs against the embedded resource and adds the rows manually.The net result is smaller, faster and more efficient. Sizoscope confirms that this PR cuts the file size of a simple application by 3.8MB, from 20.4MB to 16.6MB. Benchmark results for this in isolation are:
Separately, I noticed that my earlier PR#3309 generated two warnings when publishing because it wasn't using a fully-qualified type name. I've adjusted this.
Issues
Relates to#1942.
Relates to#1261.
Follows up#3309.
Testing
All tests continue to pass.
There's a lot of validation of the structure of the XML in the embedded resource, and this is done with debug assertions. I've treated this as hardcoded at the point of build, so haven't used exceptions for this. I can change this if needed.