Skip to content

Conversation

@github-actions
Copy link
Contributor

@github-actions github-actions bot commented Dec 19, 2023

Backport of #96007 to release/8.0-staging

/cc @MichalStrehovsky

Customer Impact

This fixes similar issue to #96167 so copying motivation from there:

Since native AOT compiler is multithreaded but we need deterministic outputs, we achieve the determinism by sorting the output before writing it to disk. The sorting ensures the same input produces the same output. However to support some first party patching scenarios, we would ideally also ensure "mostly the same input" produces "mostly the same output".

However, we didn't sort the assemblies before generating reflection metadata. This is a latent issue when it comes to reproducible builds - sorting is not needed if the set of compiled methods/types is identical. It is needed if the set varies.

Testing

Manual testing using the delta patching tool saw ~10x decrease in patch size. (This was tested together with #96167 although the impact of this one is a bit smaller.)

Risk

Low, we already use SortedSet for many other things.

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

The reflection metadata generation within the compiler follows a simple algorithm: foreach module in compilation, for each type in the module, if the type generates metadata, generate metadata for it. But it turns out the list of modules that we start with is not sorted. This is not a problem for multithreaded stability, because none of this happens in the multithreaded phase, but it is a problem for delta patching - small change in a program can perturb the metadata blob for no good reason.

This sorts the list.
@ghost
Copy link

ghost commented Dec 19, 2023

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

Issue Details

Backport of #96007 to release/8.0-staging

/cc @MichalStrehovsky

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. we will take for consideration in 8.0.x

@MichalStrehovsky MichalStrehovsky added the Servicing-consider Issue for next servicing release review label Dec 20, 2023
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 4, 2024
@rbhanda rbhanda added this to the 8.0.2 milestone Jan 4, 2024
@carlossanlop
Copy link
Contributor

@MichalStrehovsky Friendly reminder that Tuesday January 16th 4pm is the Code Complete deadline for the February Release. If all requirements are met, please merge your PR before that date and time to ensure this fix gets included in that Release.

@jeffschwMSFT jeffschwMSFT merged commit f21e249 into release/8.0-staging Jan 12, 2024
@carlossanlop carlossanlop deleted the backport/pr-96007-to-release/8.0-staging branch January 12, 2024 22:21
@github-actions github-actions bot locked and limited conversation to collaborators Feb 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-NativeAOT-coreclr Servicing-approved Approved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants