Skip to content
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.

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

Add a test for determinism #93600

Merged
merged 4 commits into from
Nov 10, 2023
Merged

Conversation

MichalStrehovsky
Copy link
Member

Resolves #73931. This checks that the result of parallel build is the same as the result of single-threaded build.

I'd like to add some more code to this so that there's more junk to compile but don't have a good idea of what to add. XmlSerializer? Suggestions welcome.

Cc @dotnet/ilc-contrib

Resolves dotnet#73931. This checks that the result of parallel build is the same as the result of single-threaded build.

I'd like to add some more code to this so that there's more junk to compile but don't have a good idea of what to add. `XmlSerializer`? Suggestions welcome.
@ghost
Copy link

ghost commented Oct 17, 2023

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

Issue Details

Resolves #73931. This checks that the result of parallel build is the same as the result of single-threaded build.

I'd like to add some more code to this so that there's more junk to compile but don't have a good idea of what to add. XmlSerializer? Suggestions welcome.

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: MichalStrehovsky
Labels:

area-NativeAOT-coreclr

Milestone: -

Comment on lines +7 to +8
using var baseline = File.OpenRead("baseline.object");
using var compare = File.OpenRead("compare.object");
Copy link
Contributor

Choose a reason for hiding this comment

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

Invoke these via reflection? Downstream, I have seen determinism issues (RyuJit-induced, since then fixed) pop up in reflection internals.

if (baseline.Length != compare.Length)
throw new Exception("Different sizes");

for (int i = 0; i < baseline.Length; i++)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (int i = 0; i < baseline.Length; i++)
long length = baseline.Length;
for (int i = 0; i < length; i++)

@jkotas
Copy link
Member

jkotas commented Oct 17, 2023

more junk to compile

Roslyn?

@MichalStrehovsky
Copy link
Member Author

more junk to compile

Roslyn?

The unfortunate part about doing that would be that we then compile Roslyn single-threaded. We want more stuff, but not too much stuff.

@MichalStrehovsky
Copy link
Member Author

I made this call into the other test that does a bunch of random things and exercises things like static virtuals, generic virtual methods, etc.

@jkotas
Copy link
Member

jkotas commented Oct 19, 2023

Looks like it found a bug:

   nativeaot\SmokeTests\Determinism\Determinism\Determinism.cmd [FAIL]
      Unhandled exception. System.Exception: Different sizes
         at Program.<Main>$(String[] args) + 0x32e

@MichalStrehovsky
Copy link
Member Author

MichalStrehovsky commented Oct 20, 2023

Looks like it found a bug:

   nativeaot\SmokeTests\Determinism\Determinism\Determinism.cmd [FAIL]
      Unhandled exception. System.Exception: Different sizes
         at Program.<Main>$(String[] args) + 0x32e

And of course the diff is in the .debug$S section because any other section would be too easy to root cause 😭

@MichalStrehovsky
Copy link
Member Author

Looks like it found a bug:

   nativeaot\SmokeTests\Determinism\Determinism\Determinism.cmd [FAIL]
      Unhandled exception. System.Exception: Different sizes
         at Program.<Main>$(String[] args) + 0x32e

And of course the diff is in the .debug$S section because any other section would be too easy to root cause 😭

I was wrong. The debug info part is just a symptom of the problem.

The problem is that codegen wasn't deterministic. Filed #93843. @SingleAccretion was right that the reflection stack is problematic.

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.

LGTM. Thanks!

@MichalStrehovsky MichalStrehovsky merged commit 5501924 into dotnet:main Nov 10, 2023
98 of 101 checks passed
@MichalStrehovsky MichalStrehovsky deleted the dettest branch November 10, 2023 07:44
@github-actions github-actions bot locked and limited conversation to collaborators Dec 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Determinism for NativeAOT
3 participants