Refactor GetManifestForRegisteredProvider to use XmlWriter by Copilot · Pull Request #2353 · microsoft/perfview

Copilot · 2026-01-16T00:52:48Z

The GetManifestForRegisteredProvider method hand-crafted XML through string concatenation and manual escaping. This refactoring replaces it with XmlWriter for better maintainability while ensuring identical output to the original implementation.

Changes

RegisteredTraceEventParser.cs

Introduced structured data model classes (EventData, TemplateData, DataField, MapData, MapEntry, StringTableEntry) to collect manifest data before serialization.
Replaced StringWriter-based XML concatenation with XmlWriter, which handles element nesting, attribute escaping, and namespace declarations automatically.
Removed manual XmlUtilities.XmlEscape calls that would have caused double-escaping when combined with XmlWriter.
Fixed a pre-existing bug where fields referencing a previously-seen map lost their map attribute (only the first field to discover a map received it).
Replaced ad-hoc delimiter-based template interning key with a structural TemplateKey class implementing IEquatable<TemplateKey> for collision-free template deduplication.
Removed unused using directives (System.Linq, Microsoft.Diagnostics.Utilities).
Fixed a typo in a comment ("Any es used so far" → "Any opcodes used so far").

RegisteredTraceEventParserTests.cs

Added GetManifestForRegisteredProvider_NoDoubleEscapedEntities: verifies no double-escaped XML entities in output and semantically compares string table entries between new and legacy
implementations.
Added GetManifestForRegisteredProvider_NewAndLegacyImplementationsProduceSameOutput: normalizes and compares full XML output against a legacy reference implementation
(Microsoft-JScript provider).
Added GetManifestForRegisteredProvider_DotNETRuntime_NewAndLegacyMatch: same comparison for the complex Microsoft-Windows-DotNETRuntime provider.
Added XML normalization helpers (NormalizeTemplateNames, RemoveNonSignificantTextNodes, SortAttributes) to handle formatting differences and template naming order between
implementations.
Embedded a copy of the legacy string-based implementation for regression comparison.

brianrob · 2026-01-16T18:56:57Z

@copilot, the new test is failing with the following error:

Testing provider: Microsoft-JScript (57277741-3638-4a4b-bdba-0ac6e45da56c)
New manifest length: 123505 chars
Legacy manifest length: 122634 chars
✓ New manifest is well-formed XML
✓ Legacy manifest is well-formed XML
✗ Normalized XML documents are different
Normalized new manifest length: 115979
Normalized legacy manifest length: 116091

First difference at position 3179:
New:    ...ts" /></keywords><tasks><task message="$(string.task_MethodRuntime)" name="MethodRuntime" value="1"><opcodes><opcode message="$(string.opcode_MethodRuntimeMethodLoad)" name="MethodLoad" value="10" /><...
Legacy: ...ts" /></keywords><tasks><task message="$(string.task_MethodRuntime)" name="MethodRuntime" value="1">
&gt;
      <opcodes><opcode message="$(string.opcode_MethodRuntimeMethodLoad)" name="MethodLoad" ...

Copilot · 2026-01-16T19:07:59Z

@copilot, the new test is failing with the following error:

...

Fixed in commit c485a84. The issue was that stringIds (like keyword_{value}, task_{name}, etc.) were not being escaped with XmlUtilities.XmlEscape() in the new implementation, while the legacy code did escape them. This caused differences in stringId deduplication and attribute values. All stringId creations now use XmlUtilities.XmlEscape() to match legacy behavior.

brianrob · 2026-02-13T02:05:17Z

@copilot, I've made a bunch of changes locally. Please update the PR description.

Copilot · 2026-02-13T02:07:57Z

@copilot, I've made a bunch of changes locally. Please update the PR description.

Updated the PR description to reflect the changes made in commits 657a433, ab5f160, and 554069f:

Key updates:

Documented the multi-version event bug fix (events storage now uses List<EventData>)
Added info about moving legacy implementation to test file
Noted the two comprehensive test cases (Microsoft-JScript and DotNETRuntime)
Documented duplication of helper methods to keep production code private
Mentioned reversion of unrelated changes to other files
Removed notes about the > formatting quirk workaround (no longer needed with improved XML normalization)

The new description better reflects the current state of the PR with your improvements.

Replace hand-crafted XML string concatenation with XmlWriter for proper XML generation. Add comparison tests against a copy of the legacy implementation to validate output equivalence. Changes: - New XmlWriter-based implementation using structured data classes (EventData, TemplateData, DataField, MapData, MapEntry, StringTableEntry) - Fix: events with multiple versions are now all preserved (List<EventData>) - Fix: proper XML escaping via XmlWriter instead of manual escaping - Legacy implementation preserved in test file for comparison testing - Tests for Microsoft-JScript, Microsoft-Windows-Ntfs, and Microsoft-Windows-DotNETRuntime providers

marklio

This seems "reasonable", but it is a little "strange". Now, instead of writing out the data, a new set of data structures is produced to represent the data and then that is transformed into the Xml. It seems like it would be simpler to replace the stringbuilder output to the equivalent XmlWriter methods. I think it probably "works", but it is definitely harder to understand what the code is doing.

marklio · 2026-03-10T22:44:42Z

src/TraceEvent/RegisteredTraceEventParser.cs

                                                    string valueName = new string((char*)(&enumBuffer[mapEntries[k].NameOffset])).Trim();
-                                                    string escapedValueName = XmlUtilities.XmlEscape(valueName);
-                                                    string stringId = XmlUtilities.XmlEscape($"map_{enumName}{valueName}");
-                                                    enumWriter.WriteLine("      <map value=\"0x{0:x}\" message=\"$(string.{1})\"/>", value, stringId);


I would have expected this to be replaced with a WriteStartElement, two WriteAttributeStrings and a WriteEndElement.

…ering Address reviewer feedback to use XmlWriter more directly. Instead of collecting into intermediate data classes (EventData, TemplateData, DataField, TemplateKey, MapData, MapEntry, StringTableEntry) and then serializing in a second pass, use XmlWriter fragment buffers to render events, templates, and maps inline during enumeration. The main document writer uses WriteRaw to inject the pre-rendered sections. - Removed 7 helper classes (EventData, TemplateData, DataField, TemplateKey, MapData, MapEntry, StringTableEntry) - Template dedup uses rendered XML content as key (collision-free) - Map dedup uses mapName key with rendered XML and display name - Preserved map-reuse bug fix (display name lookup for all map refs) - TaskInfo retained (pre-existing, used for task/opcode tracking) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Add tracking of string table entries that contain XML-special characters in the semantic comparison loop. This surfaces whether the test data actually exercises the escaping path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

marklio

Looks reasonable. Took me a while to wrap my head around the up-front fragment rendering and realize this is what was happening before as well.

Copilot AI assigned Copilot and brianrob Jan 16, 2026

Copilot started work on behalf of brianrob January 16, 2026 00:53 View session

Copilot AI changed the title ~~[WIP] Refactor GetManifestForRegisteredProvider to use XmlWriter~~ Refactor GetManifestForRegisteredProvider to use XmlWriter Jan 16, 2026

Copilot finished work on behalf of brianrob January 16, 2026 01:07

Copilot AI requested a review from brianrob January 16, 2026 01:07

Copilot started work on behalf of brianrob January 16, 2026 18:57 View session

Copilot finished work on behalf of brianrob January 16, 2026 19:10

Copilot started work on behalf of brianrob January 16, 2026 19:35 View session

Copilot finished work on behalf of brianrob January 16, 2026 19:39

Copilot started work on behalf of brianrob February 13, 2026 02:05 View session

Copilot finished work on behalf of brianrob February 13, 2026 02:08

brianrob force-pushed the copilot/refactor-xml-output-using-xmlwriter branch from 554069f to d08077e Compare February 13, 2026 02:15

brianrob added 2 commits February 22, 2026 14:33

Address review feedback.

93caf69

Address review feedback.

f84a796

brianrob marked this pull request as ready for review February 23, 2026 20:48

brianrob requested review from StephenMolloy, cincuranet, leculver, marklio and mconnew as code owners February 23, 2026 20:48

marklio reviewed Mar 10, 2026

View reviewed changes

brianrob and others added 2 commits March 10, 2026 17:22

marklio approved these changes Mar 11, 2026

View reviewed changes

brianrob merged commit f9873e6 into main Mar 11, 2026
9 checks passed

brianrob deleted the copilot/refactor-xml-output-using-xmlwriter branch March 11, 2026 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor GetManifestForRegisteredProvider to use XmlWriter#2353

Refactor GetManifestForRegisteredProvider to use XmlWriter#2353
brianrob merged 5 commits intomainfrom
copilot/refactor-xml-output-using-xmlwriter

Copilot AI commented Jan 16, 2026 •

edited by brianrob

Loading

Uh oh!

brianrob commented Jan 16, 2026

Uh oh!

Copilot AI commented Jan 16, 2026

Uh oh!

brianrob commented Feb 13, 2026

Uh oh!

Copilot AI commented Feb 13, 2026

Uh oh!

marklio left a comment

Uh oh!

marklio Mar 10, 2026

Uh oh!

marklio left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Copilot AI commented Jan 16, 2026 • edited by brianrob Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

brianrob commented Jan 16, 2026

Uh oh!

Copilot AI commented Jan 16, 2026

Uh oh!

brianrob commented Feb 13, 2026

Uh oh!

Copilot AI commented Feb 13, 2026

Uh oh!

marklio left a comment

Choose a reason for hiding this comment

Uh oh!

marklio Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

marklio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Jan 16, 2026 •

edited by brianrob

Loading