Skip to content

Fix Metadata aggregator: removing cumulative sum for GUIDs heap offset #115268

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Fix Metadata aggregator: removing cumulative sum for GUIDs heap offset #115268

wants to merge 20 commits into from
+70 −2

Conversation

@fabrimaz
Copy link

@fabrimaz fabrimaz commented May 3, 2025 •
edited
Loading

Removing cumulative sum for GUID heap offsets
Fixes #113910

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 3, 2025
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-reflection-metadata
See info in area-owners.md if you want to be subscribed.

@fabrimaz
Copy link
Author

fabrimaz commented May 3, 2025

Run unit-tests on this library:

=== TEST EXECUTION SUMMARY ===
System.Reflection.Metadata.Tests Total: 852, Errors: 0, Failed: 0, Skipped: 1, Time: 5,376s

@fabrimaz fabrimaz mentioned this pull request May 3, 2025
Open
@build-analysis build-analysis bot mentioned this pull request May 3, 2025
Open
tmat
tmat reviewed May 6, 2025
View reviewed changes
...ries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/MetadataAggregator.cs
stringSizes[r + 1] = stringSizes[r] + deltaReaders[r].GetHeapSize(HeapIndex.String);
blobSizes[r + 1] = blobSizes[r] + deltaReaders[r].GetHeapSize(HeapIndex.Blob);
guidSizes[r + 1] = guidSizes[r] + deltaReaders[r].GetHeapSize(HeapIndex.Guid) / guidSize;
guidSizes[r + 1] = deltaReaders[r].GetHeapSize(HeapIndex.Guid) / guidSize;
Copy link
Member

Choose a reason for hiding this comment

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

The comment on line https://github.com/dotnet/runtime/pull/115268/files#diff-463bcba9d713f9c49493793a38b92567be4f344112abe18ebd08a34d3a5a1659R13 should be updated

@tmat
Copy link
Member

tmat commented May 6, 2025

It'd be good to have some tests that demonstrate correctness. There are some existing tests for MetadataAggregator, so you can update them. Make sure the test fails without your change and passes with the change.

@fabrimaz
Copy link
Author

fabrimaz commented May 10, 2025 •
edited
Loading

It'd be good to have some tests that demonstrate correctness. There are some existing tests for MetadataAggregator, so you can update them. Make sure the test fails without your change and passes with the change.

Hello! The linked comment was updated :)

The method "CalculateHeapSizes(..)" I edited is private and its result is assigned to a private field directly in the constructor of the class.
Even if I try to unit-test that constructor, I wouldn't be able to easily access the private field from unit-test class to assert its values.

What would you suggest? Should I split the mentioned method in a separate class to enhance testability?

Or else making the method 'internal'?

@fabrimaz fabrimaz requested a review from tmat May 10, 2025 07:56
@build-analysis build-analysis bot mentioned this pull request May 10, 2025
Open
@fabrimaz
Copy link
Author

Ping @tmat

@fabrimaz
Copy link
Author

fabrimaz commented Jul 6, 2025

ping @tmat

@tmat
Copy link
Member

tmat commented Jul 7, 2025

See https://github.com/dotnet/runtime/blob/main/src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/MetadataAggregatorTests.cs

@ericstj
Copy link
Member

ericstj commented Jul 7, 2025

@fabrimaz as @tmat mentions the action here is to add a test to https://github.com/dotnet/runtime/blob/main/src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/MetadataAggregatorTests.cs. It looks like there are already tests that call TestGenerationHandle which should be able to observe the broken behavior called out in #113910.

@fabrimaz
Copy link
Author

fabrimaz commented Aug 19, 2025 •
edited
Loading

@ericstj I might be completely wrong but feels like all the existing unit-tests are building _heapSize in a simplified way through a fake internal ctor, hence not replicating our 'issue'.

To unit-test our behaviour, I need to use at least a baseReader and a list of MetadataReader deltas.
Going in that direction and keeping you posted
@tmat

@fabrimaz
Copy link
Author

By chance, do you have any clue on how to create a minimal MetadataReader with an allocated GUID? Couldn't find a good option among existing unit-tests for MetadataReader

@fabrimaz
Copy link
Author

@dotnet-policy-service agree

@fabrimaz
Copy link
Author

@ericstj @tmat
glad to tell that I managed to build meaningful unit-tests for the specific case.
Needed to create MetadataReader manually to fully test the behaviour.

Please, give it a review and let me know your opinion about :)

tmat
tmat reviewed Sep 2, 2025
View reviewed changes
tmat
tmat reviewed Sep 2, 2025
View reviewed changes
src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/MetadataAggregatorTests.cs Outdated
// to avoid minimal flag exception.
reader.TableRowCounts[(int)TableIndex.EncMap] = 1;
reader.IsMinimalDelta = true;
return reader;
Copy link
Member

Choose a reason for hiding this comment

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

You're returning reader that accesses memory at pointer p but the target array can move once fixed block is exited. You need to keep the array pinned while using the reader.

Copy link
Author

Choose a reason for hiding this comment

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

I refactored according to your suggestions

This was referenced Sep 2, 2025
Open
Open
Open
tmat
tmat reviewed Sep 5, 2025
View reviewed changes
src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/MetadataAggregatorTests.cs

// GUID-heap allocation shouldn't be cumulative, since GUIDs are copied among generations.
// The delta-reader above need indeed a single GUID allocation in each gen.
AssertExtensions.Throws<ArgumentException>("handle", () => TestGenerationHandle(aggregator, MetadataTokens.GuidHandle(2), expectedHandle: MetadataTokens.GuidHandle(2), expectedGeneration: 0));
Copy link
Member

Choose a reason for hiding this comment

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

We should be able to read 3 guids since there is one in baseline, and one in each delta, no?

Copy link
Author

@fabrimaz fabrimaz Sep 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@tmat in a real case yes, not in this test though.
Our new implementation removes cumulative sum and relies on each delta-reader GUID heap size, relying on deltareaders the copying of previous GUIDs and adding of existing new ones.

With new implementation, each generation here allocates a single GUID, hence only that one at index 1 could be read.
Tried to allocate more GUIDs on manually created delta-readers but couldn't find a working example.

Copy link
Author

Choose a reason for hiding this comment

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

With cumulative it was
Gen0: #1
Gen1: #1 #2
Gen2: #1 #2 #3

With new implementation
Gen0: #1
Gen1: #1
Gen2: #1

It's ok because all existing GUIDs are actually copied in next generation of metadata reader normally.

@fabrimaz fabrimaz requested a review from tmat September 8, 2025 15:56
@fabrimaz
Copy link
Author

Ping @tmat @ericstj

1 similar comment
@fabrimaz
Copy link
Author

fabrimaz commented Oct 3, 2025

Ping @tmat @ericstj

@ericstj ericstj requested a review from Copilot October 6, 2025 18:01
Copilot AI reviewed Oct 6, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

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 fixes the GUID heap offset calculation in the MetadataAggregator by removing cumulative sum aggregation for GUID heap sizes. Unlike other heaps, GUIDs are copied across generations rather than being cumulative, so their heap sizes should represent the actual number of available GUIDs in each generation, not a running total.

  • Removes cumulative sum calculation for GUID heap offsets in MetadataAggregator
  • Updates documentation to clarify the different treatment of GUID heap sizes vs other heap types
  • Adds comprehensive test coverage for the corrected GUID heap size behavior

Reviewed Changes

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

File Description
src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/MetadataAggregator.cs Updates GUID heap size calculation to use direct values instead of cumulative sum and adds clarifying documentation
src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma335/MetadataAggregatorTests.cs Adds new test method and helper methods to verify correct GUID heap size behavior, plus additional using statements

fabrimaz and others added 2 commits October 6, 2025 22:42
@fabrimaz
Update src/libraries/System.Reflection.Metadata/tests/Metadata/Ecma33…
fe32d04 
…5/MetadataAggregatorTests.cs


Copilot suggestion

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@fabrimaz
Merge branch 'main' into fix/113910
f7777b1
@fabrimaz
Copy link
Author

fabrimaz commented Oct 7, 2025

hello,
fixed Copilot remarks
@ericstj @tmat

@fabrimaz
Copy link
Author

@ericstj @tmat
Is this fix still needed?

@ericstj
Copy link
Member

ericstj commented Oct 21, 2025

Yeah, I asked @tmat about it and he said he'll get back to it, but it's not a top priority at the moment I believe because a workaround is in place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@tmat tmat Awaiting requested review from tmat

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

area-System.Reflection.Metadata community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

MetadataAggregator.GetGenerationHandle returns incorrect results for Guid heap handles

4 participants

@fabrimaz @tmat @ericstj @steveharter