Skip to content

Improve method body interning and deduplication logic #117080

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
Merged
MichalStrehovsky merged 4 commits into from
Jul 1, 2025
Merged

Improve method body interning and deduplication logic #117080

MichalStrehovsky merged 4 commits into from
Jul 1, 2025

Conversation

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky MichalStrehovsky commented Jun 27, 2025 •
edited
Loading

  • Enhanced ObjectDataInterner to perform multiple iterations of interning.
  • Consider two same MethodReadOnlyDataNode the same.

Cc @dotnet/ilc-contrib

@MichalStrehovsky
Improve method body interning and deduplication logic
c52f7a2 
* Enhanced ObjectDataInterner to perform multiple iterations of interning.
* Consider two same `MethodReadOnlyDataNode` the same.
Copilot AI review requested due to automatic review settings June 27, 2025 09:36
@dotnet-policy-service
Copy link
Contributor

dotnet-policy-service bot commented Jun 27, 2025

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Copilot AI reviewed Jun 27, 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 enhances the object data interning process by running multiple interning iterations and treats identical MethodReadOnlyDataNode instances as duplicates, while also adding logic to skip emitting deduplicated read-only data nodes.

  • Run interning in a loop until no new mappings are found
  • Extend MethodInternComparer to consider MethodReadOnlyDataNode content equality
  • Add ShouldSkipEmittingObjectNode to avoid emitting already-interned read-only data

Reviewed Changes

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

File Description
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectDataInterner.cs Introduce a loop over interning passes and update comparer to handle method RO data
src/coreclr/tools/Common/Compiler/DependencyAnalysis/MethodReadOnlyDataNode.cs Add override to skip emitting deduplicated MethodReadOnlyDataNode
Comments suppressed due to low confidence (2)

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectDataInterner.cs:31

  • The new iterative interning loop in EnsureMap should be covered by unit tests to verify that duplicates are progressively deduplicated across multiple iterations.
            do

src/coreclr/tools/Common/Compiler/DependencyAnalysis/MethodReadOnlyDataNode.cs:21

  • Add unit tests for ShouldSkipEmittingObjectNode to ensure that read-only data nodes are correctly skipped when they have been interned elsewhere.
        public override bool ShouldSkipEmittingObjectNode(NodeFactory factory)

@github-actions github-actions bot mentioned this pull request Jun 27, 2025
Closed
@MichalStrehovsky
Copy link
Member Author

MichalStrehovsky commented Jun 27, 2025

Size statistics

Pull request #117080

Project Size before Size after Difference
TodosApi-linux 23100480 23010368 -90112
TodosApi-windows 20862464 20794368 -68096
avalonia.app-linux 18573840 18520592 -53248
avalonia.app-windows 16511488 16473088 -38400
hello-linux 1262480 1258384 -4096
hello-minimal-linux 1115032 1110936 -4096
hello-minimal-windows 861184 858624 -2560
hello-windows 1007104 1004544 -2560
kestrel-minimal-linux 5234104 5213624 -20480
kestrel-minimal-windows 4779520 4760576 -18944
reflection-linux 1817248 1809056 -8192
reflection-windows 1544192 1538560 -5632
webapiaot-linux 8981096 8944232 -36864
webapiaot-windows 8241664 8207872 -33792
winrt-component-full-windows 5179392 5147136 -32256
winrt-component-minimal-windows 1732608 1717760 -14848

@build-analysis build-analysis bot mentioned this pull request Jun 27, 2025
Closed
@jkotas
Copy link
Member

jkotas commented Jun 27, 2025

perform multiple iterations of interning.

How many iterations do we typically get for larger apps like TodosApi-linux?

jkotas
jkotas approved these changes Jun 27, 2025
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Nice!

@MichalStrehovsky
Copy link
Member Author

MichalStrehovsky commented Jul 1, 2025

perform multiple iterations of interning.

How many iterations do we typically get for larger apps like TodosApi-linux?

TodosApi gets 6 iterations:

Iteration done, 14957 folded
Iteration done, 15328 folded
Iteration done, 15382 folded
Iteration done, 15391 folded
Iteration done, 15398 folded
Iteration done, 15398 folded

@MichalStrehovsky MichalStrehovsky merged commit 336855a into dotnet:main Jul 1, 2025
95 of 97 checks passed
@MichalStrehovsky MichalStrehovsky deleted the morefolding branch July 1, 2025 10:11
@jkotas jkotas added the tenet-performance Performance related issue label Jul 3, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Aug 2, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotas jkotas jkotas approved these changes

Copilot code review Copilot Copilot left review comments

Assignees

@MichalStrehovsky MichalStrehovsky

Labels

area-NativeAOT-coreclr tenet-performance Performance related issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MichalStrehovsky @jkotas