Skip to content

Conversation

@LoopedBard3
Copy link
Member

Adds MonoAOTLLVM scenario support to the local testing script. Because the MonoAOTLLVM toolchain is only setup to run one BDN test set per invocation, the tool runs multiple which then can be compared using the ResultsComparer tool.

@LoopedBard3 LoopedBard3 added the enhancement New feature or request label Sep 12, 2023
@LoopedBard3 LoopedBard3 self-assigned this Sep 12, 2023
@LoopedBard3 LoopedBard3 marked this pull request as ready for review September 12, 2023 20:37
@matouskozak
Copy link
Member

Tested locally on osx-arm64 and I'm getting. It looks similar to dotnet/runtime#91819. Is it possible that it is a Mono issue?

[2023/09/15 15:53:45][INFO] 
[2023/09/15 15:53:45][INFO]   Mono Warning: --llvm not enabled in this runtime.
[2023/09/15 15:53:45][INFO]   Mono Ahead of Time compiler - compiling assembly /Users/matouskozk/work/repos/performance/artifacts/bin/MicroBenchmarks/Release/net8.0/Job-SWNCKQ/bin/net8.0/osx-arm64/publish/System.Private.CoreLib.dll
[2023/09/15 15:53:45][INFO]   AOTID 7F041A4C-D241-DDA9-C9C5-07C9162C2327
[2023/09/15 15:53:45][INFO]   * Assertion at /Users/matouskozk/work/repos/baseline_runtime/runtime/src/mono/mono/mini/mini-arm64.c:3915, condition `mono_class_value_size (ins->klass, NULL) == 16' not met
``

@matouskozak
Copy link
Member

matouskozak commented Sep 16, 2023

Tested locally on osx-arm64 and I'm getting. It looks similar to dotnet/runtime#91819. Is it possible that it is a Mono issue?

[2023/09/15 15:53:45][INFO] 
[2023/09/15 15:53:45][INFO]   Mono Warning: --llvm not enabled in this runtime.
[2023/09/15 15:53:45][INFO]   Mono Ahead of Time compiler - compiling assembly /Users/matouskozk/work/repos/performance/artifacts/bin/MicroBenchmarks/Release/net8.0/Job-SWNCKQ/bin/net8.0/osx-arm64/publish/System.Private.CoreLib.dll
[2023/09/15 15:53:45][INFO]   AOTID 7F041A4C-D241-DDA9-C9C5-07C9162C2327
[2023/09/15 15:53:45][INFO]   * Assertion at /Users/matouskozk/work/repos/baseline_runtime/runtime/src/mono/mono/mini/mini-arm64.c:3915, condition `mono_class_value_size (ins->klass, NULL) == 16' not met
``

I tested the script on Ubuntu 22.04 x64 (WSL) and it looks to run correctly.

Edit: Upon more detailed inspection I believe that while the script runs with Ubuntu on x64 it doesn't use LLVM backend but mini.

@matouskozak
Copy link
Member

matouskozak commented Sep 18, 2023

Tested locally on osx-arm64 and I'm getting. It looks similar to dotnet/runtime#91819. Is it possible that it is a Mono issue?

[2023/09/15 15:53:45][INFO] 
[2023/09/15 15:53:45][INFO]   Mono Warning: --llvm not enabled in this runtime.
[2023/09/15 15:53:45][INFO]   Mono Ahead of Time compiler - compiling assembly /Users/matouskozk/work/repos/performance/artifacts/bin/MicroBenchmarks/Release/net8.0/Job-SWNCKQ/bin/net8.0/osx-arm64/publish/System.Private.CoreLib.dll
[2023/09/15 15:53:45][INFO]   AOTID 7F041A4C-D241-DDA9-C9C5-07C9162C2327
[2023/09/15 15:53:45][INFO]   * Assertion at /Users/matouskozk/work/repos/baseline_runtime/runtime/src/mono/mono/mini/mini-arm64.c:3915, condition `mono_class_value_size (ins->klass, NULL) == 16' not met
``

I re-tested the script on osx-arm64 with clang-16 and I'm getting the same error. It looks like it built without enabled LLVM, report from mono-aot-cross --llvm --version:

Mono Warning: --llvm not enabled in this runtime.
Mono JIT compiler version 9.0.0.0 (42.42.42.42424)
Copyright (C) Novell, Inc, Xamarin Inc and Contributors. www.mono-project.com
        TLS:           
        SIGSEGV:       normal
        Architecture:  arm64
        Misc:          softdebug 
        Interpreter:   yes
        LLVM:          supported, not enabled.
        Suspend:       hybrid
        GC:            sgen (concurrent by default)

@LoopedBard3 LoopedBard3 marked this pull request as draft September 18, 2023 17:32
@matouskozak
Copy link
Member

matouskozak commented Sep 22, 2023

After the last change which added /p:MonoAOTEnableLLVM=true and /p:MonoBundleLLVMOptimizer=true flags for arm64. I can run MonoAOTLLVM on osx-arm64 machine and mono-aot-cross --llvm --version reports:

Mono JIT compiler version 9.0.0.0 (42.42.42.42424)
Copyright (C) Novell, Inc, Xamarin Inc and Contributors. www.mono-project.com
        TLS:           
        SIGSEGV:       normal
        Architecture:  arm64
        Misc:          softdebug 
        Interpreter:   yes
        LLVM:          yes(1600)
        Suspend:       hybrid
        GC:            sgen (concurrent by default)

However, I'm concerned about the Ubuntu scenario where mono-aot-cross --llvm --version reports:

Mono Warning: --llvm not enabled in this runtime.
Mono JIT compiler version 9.0.0.0 (42.42.42.42424)
Copyright (C) Novell, Inc, Xamarin Inc and Contributors. www.mono-project.com
        TLS:
        SIGSEGV:       normal
        Architecture:  amd64
        Misc:          softdebug
        Interpreter:   yes
        LLVM:          supported, not enabled.
        Suspend:       hybrid
        GC:            sgen (concurrent by default)

and based on some testing I believe the AOT is running with mini not llvm backend. Any thoughts on what might be missing? Could it be related to recent issues with LLVM on Ubuntu? I'm unsure whether this is a x64 vs arm64 or Linux vs Mac issue with MonoAOTLLVM (I suspect the latter).
Edit: I manage to get llvm enabled on Ubuntu by adding /p:MonoEnableLLVM=true and /p:MonoAOTEnableLLVM=true build flags. (/p:MonoBundleLLVMOptimizer=true seems to be optional in this scenario)

How do we test that the machines running in perf lab are doing AOT-llvm and not using mini? Could we verify by using mono-aot-cross --llvm --version on the lab machine (or other way) that we actually do AOT-llvm? @kotlarmilos

@matouskozak
Copy link
Member

Not related to this PR but I found that when running MonoJIT with --commits flag the build fails on building bash build.sh -subset libs.pretest -configuration Release -os osx -arch arm64 -bl -testscope innerloop /p:RuntimeFlavor=mono /p:RuntimeArtifactsPath=./runtime/artifacts/bin/mono/osx.arm64.Release

The error is

/Users/matouskozk/work/repos/performance/scripts/runtime/eng/liveBuilds.targets(63,5): error : The Mono artifacts path does not exist '/Users/matouskozk/work/repos/performance/scripts/runtime/eng/runtime/artifacts/bin/mono/osx.arm64.Release/'. The 'mono' subset must be built before building this project. Configuration: 'Release'. To use a different configuration, specify the 'RuntimeConfiguration' property.

I think that the artifacts get place in .runtime/artifacts... and not into ./runtime/eng/runtime/artifacts.... However, this doesn't seem to cause problems when using --local-test-repo instead of --commits.

@fanyang-mono
Copy link
Member

It would be nice to include a brief instruction on how to use this script. I see that common usage was mentioned in the comment at the beginning of the script. It would be very helpful to also list all the possible switches and what they do.

@fanyang-mono
Copy link
Member

When using the script, I noticed in the log, it said

[INFO] Killing any running dotnet, vstest, or msbuild processes... (ignore system cannot find path specified)

This seems to be a overkill for local runs.

@kotlarmilos
Copy link
Member

kotlarmilos commented Sep 22, 2023

How do we test that the machines running in perf lab are doing AOT-llvm and not using mini? Could we verify by using mono-aot-cross --llvm --version on the lab machine (or other way) that we actually do AOT-llvm? @kotlarmilos

Yes, I think it is a good idea. Let's fix the naming and add mono-aot-cross --llvm --version command in a follow-up PR.

Please post your findings when you get a chance.

@LoopedBard3
Copy link
Member Author

How do we test that the machines running in perf lab are doing AOT-llvm and not using mini? Could we verify by using mono-aot-cross --llvm --version on the lab machine (or other way) that we actually do AOT-llvm? @kotlarmilos

Yes, I think it is a good idea. Let's fix the naming and add mono-aot-cross --llvm --version command in a follow-up PR.

Please post your findings when you get a chance.

If it is based on the mono-aot-cross executable, should we be able to pull down the built artifact and test on a like system, or is there system specific things that we would need to check for on the specific machines? If it can be tested locally, I can pull down the build mono-aot-cross to wsl and get the output.

@kotlarmilos
Copy link
Member

If it is based on the mono-aot-cross executable, should we be able to pull down the built artifact and test on a like system, or is there system specific things that we would need to check for on the specific machines? If it can be tested locally, I can pull down the build mono-aot-cross to wsl and get the output.

Yes, I think we can retrieve it from artifacts and use it on linux-x64.

… to use should be the ones present in the Versions.props, otherwise built artifacts end up with unexpected paths causing failing builds. As such, also removed the framework from the artifact path for this reason. Removed the cross arguments as they should not be necessary for local builds.
@fanyang-mono
Copy link
Member

When using the script, I noticed in the log, it said

[INFO] Killing any running dotnet, vstest, or msbuild processes... (ignore system cannot find path specified)

This seems to be a overkill for local runs.

I understand that this needs to be done to ensure the accuracy of the results. However, this prevented me from doing other work, such as building other repos. One possible solution to this issue could be running the microbenchmarks inside another vm or a codespace or a docker container or some other controlled environment for use cases of validating a certain commit hash for causing performance changes.

@fanyang-mono
Copy link
Member

It would be nice for this script to support running microbenchmarks from a local dotnet/performance repo. It is beneficial for use cases like adding new microbenchmarks and editing existing microbenchmarks.

@LoopedBard3
Copy link
Member Author

It would be nice for this script to support running microbenchmarks from a local dotnet/performance repo. It is beneficial for use cases like adding new microbenchmarks and editing existing microbenchmarks.

Do you mean in a different local dotnet/performance repo? Since the current setup uses the scripts in the dotnet/performance repo that it is being run from, local changes should be testable/reflected properly when made to the same repo as the script. Please let me know if this has not been your experience or if you mean some other setup.

@fanyang-mono
Copy link
Member

It would be nice for this script to support running microbenchmarks from a local dotnet/performance repo. It is beneficial for use cases like adding new microbenchmarks and editing existing microbenchmarks.

Do you mean in a different local dotnet/performance repo? Since the current setup uses the scripts in the dotnet/performance repo that it is being run from, local changes should be testable/reflected properly when made to the same repo as the script. Please let me know if this has not been your experience or if you mean some other setup.

You are right. I confused myself. Please ignore this comment.

Copy link
Member

@matouskozak matouskozak left a comment

Choose a reason for hiding this comment

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

LGTM!

As mentioned by @fanyang-mono, for future work I would consider removing the kill all dotnet processes.

Additionally, when running with admin privileges, it would be nice that only the microbenchmarks are run with higher privileges and building of runtime is done with standard user privileges. This matters when working with local repo as you usually must then clean everything so that you can rebuild in normal user mode.

@LoopedBard3 LoopedBard3 marked this pull request as ready for review October 2, 2023 21:50
@kotlarmilos kotlarmilos self-requested a review October 3, 2023 10:19
Copy link
Member

@kotlarmilos kotlarmilos left a comment

Choose a reason for hiding this comment

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

Thank you!

@LoopedBard3 LoopedBard3 enabled auto-merge (squash) October 3, 2023 17:59
@LoopedBard3 LoopedBard3 disabled auto-merge October 3, 2023 17:59
@LoopedBard3 LoopedBard3 merged commit 77df656 into dotnet:main Oct 3, 2023
@LoopedBard3 LoopedBard3 deleted the AddMonoAOTLLVMToLocalScript branch October 3, 2023 18:01
@fanyang-mono
Copy link
Member

I've opened an issue (#3385) to captured most of the suggestion, which are not included in this PR. Feel free to add more, if I missed anything.

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants