Skip to content

Port TypeMapObjectsXmlFile.Import to XmlReader streaming - #11126

Merged
jonathanpeppers merged 5 commits into
mainfrom
jonathanpeppers/dev-peppers-typemap-xmlreader
Apr 17, 2026
Merged

Port TypeMapObjectsXmlFile.Import to XmlReader streaming#11126
jonathanpeppers merged 5 commits into
mainfrom
jonathanpeppers/dev-peppers-typemap-xmlreader

Conversation

@jonathanpeppers

@jonathanpeppersjonathanpeppers commented Apr 16, 2026

Copy link
Copy Markdown
Member

Summary

Replace XDocument.Load (full DOM parse per assembly) with forward-only XmlReader streaming in the Import path of TypeMapObjectsXmlFile. This avoids allocating the entire XML DOM tree for each .typemap.xml file during the build.

Changes

  • Replace Import, ImportDebugData, ImportReleaseData methods to use XmlReader
  • Add ReadDebugEntries, ReadReleaseEntries, ReadReleaseScratchEntries, ReadReleaseEntry helper methods for streaming reads
  • Use depth-tracking to robustly detect container end-elements
  • Remove unused System.Linq, System.Xml.Linq, NuGet.Packaging usings
  • Export side (XmlWriter) is unchanged

@jonathanpeppers
Port TypeMapObjectsXmlFile.Import to XmlReader streaming
ff23816
Replace XDocument.Load (full DOM parse per assembly) with forward-only
XmlReader streaming in the Import path. This avoids allocating the
entire XML DOM tree for each .typemap.xml file.
- Replace Import, ImportDebugData, ImportReleaseData methods to use XmlReader
- Add ReadDebugEntries, ReadReleaseEntries, ReadReleaseScratchEntries,
ReadReleaseEntry helper methods for streaming reads
- Use depth-tracking to robustly detect container end-elements
- Remove unused System.Linq, System.Xml.Linq, NuGet.Packaging usings
- Export side (XmlWriter) is unchanged
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings April 16, 2026 15:06
CopilotAI reviewed Apr 16, 2026
View reviewed changes

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors TypeMapObjectsXmlFile.Import to use forward-only XmlReader streaming instead of XDocument.Load, reducing per-assembly allocations during typemap XML import in the build tasks.

Changes:

  • Replaced DOM-based import (XDocument/XElement) with streaming XmlReader parsing.
  • Added streaming helpers to read debug/release entries with depth-based container termination.
  • Removed now-unused System.Linq, System.Xml.Linq, and NuGet.Packaging usings.

Comment threadsrc/Xamarin.Android.Build.Tasks/Utilities/TypeMapObjectsXmlFile.cs Outdated
Comment threadsrc/Xamarin.Android.Build.Tasks/Utilities/TypeMapObjectsXmlFile.cs Outdated
Comment threadsrc/Xamarin.Android.Build.Tasks/Utilities/TypeMapObjectsXmlFile.cs Outdated
Comment threadsrc/Xamarin.Android.Build.Tasks/Utilities/TypeMapObjectsXmlFile.cs Outdated
jonathanpeppersand others added 2 commits April 16, 2026 10:16
@jonathanpeppers
fixup: reorder methods to minimize diff noise
da7fc28
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@jonathanpeppers
fixup: match original error behavior, improve error messages
c5534f1
- Use Guid.Parse / Convert.ToBoolean / Convert.ChangeType to throw on
invalid values (matching original GetAttributeOrDefault behavior)
- Add GetAttributeOrDefault<T> helper for XmlReader
- Include filename in error messages
- Check IsNullOrWhiteSpace for 'type' attribute (matching GetRequiredAttribute)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI reviewed Apr 16, 2026
View reviewed changes

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

Comment threadsrc/Xamarin.Android.Build.Tasks/Utilities/TypeMapObjectsXmlFile.cs
Comment threadsrc/Xamarin.Android.Build.Tasks/Utilities/TypeMapObjectsXmlFile.cs Outdated
@jonathanpeppers
fixup: match original behavior for mvid-bytes and duplicate keys
679a791
- Use GetAttributeOrDefault for mvid-bytes to handle whitespace the
same way as the original GetAttributeOrDefault<T> on XElement
- Use entries.Add() instead of entries[key] to throw on duplicate keys,
matching the original NuGet.Packaging AddRange behavior
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jonathanpeppers
jonathanpeppers commented Apr 16, 2026
View reviewed changes

@jonathanpeppersjonathanpeppers left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 AI Review Summary

Verdict: ✅ LGTM

Found 3 items: 1 ⚠️ warning, 2 💡 suggestions

  • ⚠️Nullable: Use .IsNullOrWhiteSpace () extension method per repo convention (4 occurrences)
  • 💡 Code organization: foundJniNativeRegistration parsing can use the new GetAttributeOrDefault helper for consistency
  • 💡 Performance: Convert.ChangeType should specify CultureInfo.InvariantCulture for round-trip safety

👍 Clean port from XDocument to XmlReader streaming. The depth-tracking pattern is correct and robust. Good removal of System.Linq, System.Xml.Linq, and NuGet.Packaging usings. Export side is correctly left unchanged.

Review generated by android-reviewer from review guidelines.

Comment threadsrc/Xamarin.Android.Build.Tasks/Utilities/TypeMapObjectsXmlFile.cs Outdated
Comment threadsrc/Xamarin.Android.Build.Tasks/Utilities/TypeMapObjectsXmlFile.cs Outdated
Comment threadsrc/Xamarin.Android.Build.Tasks/Utilities/TypeMapObjectsXmlFile.cs Outdated
@jonathanpeppers
fixup: use extension methods, GetAttributeOrDefault, InvariantCulture
15e511a
- Use .IsNullOrWhiteSpace() extension methods for NRT flow analysis
- Reuse GetAttributeOrDefault for found-jni-native-registration
- Pass CultureInfo.InvariantCulture to Convert.ChangeType for
round-trip safety with Export side
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival
simonrozsival approved these changes Apr 17, 2026
View reviewed changes

@simonrozsivalsimonrozsival left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes look reasonable, CI failures are unrelated.

@jonathanpeppers
jonathanpeppers merged commit 32aed41 into mainApr 17, 2026
5 of 6 checks passed
@jonathanpeppers
jonathanpeppers deleted the jonathanpeppers/dev-peppers-typemap-xmlreader branch April 17, 2026 13:43
@simonrozsivalsimonrozsival mentioned this pull request Apr 24, 2026
Closed
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 18, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Copilot code reviewCopilot
Copilot review effort, defaults to Lite
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@simonrozsivalsimonrozsivalsimonrozsival approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jonathanpeppers@simonrozsival