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

Delete CMake infrastructure for crossgen1 and remove dead code directories #57614

Merged
merged 9 commits into from
Aug 18, 2021

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Aug 17, 2021

I've split this into individual commits that should each build successfully and can be reviewed either individually or all together.

Remove all CMake targets that were only used to compose crossgen1.

Delete any folders where the files within were only used to compile crossgen1-only CMake targets.

Delete all CMake code based on the CROSSGEN_COMPONENT CMake target property.

The following defines are now never defined and all blocks guarded by them can be considered dead code. We can now

  • Remove code guarded by CROSSGEN_COMPILE
  • Remove code guarded by FEATURE_NATIVE_IMAGE_GENERATION
  • Remove code guarded by FEATURE_READYTORUN_COMPILER (outside of the coreclr/jit directory. It is still used there)
  • Remove code guarded by FEATURE_PREJIT
  • Remove code guarded by FEATURE_NGEN_RELOCS_OPTIMIZATIONS

Contributes to #54129 and #53007

cc: @dotnet/crossgen-contrib @AaronRobinsonMSFT @elinor-fung @jkotas @agocke

Code references have not been removed.

The following defines are now never defined and all blocks guarded by them can be considered dead code:

- [ ] CROSSGEN_COMPILE
- [ ] FEATURE_NATIVE_IMAGE_GENERATION
- [ ] FEATURE_READYTORUN_COMPILER (outside of the coreclr/jit directory)
@jkoritzinsky jkoritzinsky added area-CrossGen/NGEN-coreclr tenet-build-performance Impacts build time: official, developer or CI labels Aug 17, 2021
@jkoritzinsky jkoritzinsky added this to the 7.0.0 milestone Aug 17, 2021
Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

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

As my former lead Slava used to say, red is always good. Thank you!

@ghost
Copy link

ghost commented Aug 17, 2021

Hello @jkoritzinsky!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 10 minutes, a condition that will be fulfilled in about 6 minutes. No worries though, I will be back when the time is right! 😉

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@@ -1,168 +0,0 @@
::mscorlib
Copy link
Member

@am11 am11 Aug 17, 2021

Choose a reason for hiding this comment

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

nit: also found some crossgen1 hits in few other places:

$ git grep -li crossgen1 :/

docs/design/coreclr/botr/vectors-and-intrinsics.md
src/coreclr/crossgen-corelib.proj
src/coreclr/tools/aot/ILCompiler.Diagnostics/PerfMapWriter.cs
src/coreclr/tools/aot/ILCompiler.ReadyToRun/ObjectWriter/SymbolFileBuilder.cs
src/coreclr/tools/r2rtest/Crossgen2Runner.cs
src/coreclr/tools/r2rtest/CrossgenRunner.cs
src/tasks/Crossgen2Tasks/PrepareForReadyToRunCompilation.cs
src/tasks/Crossgen2Tasks/RunReadyToRunCompiler.cs
src/tests/readytorun/crossboundarylayout/crossboundarytests.cmd

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can remove those occurrences in a future PR. I want to keep this one focused on the native build for simplicity.

Copy link
Member

Choose a reason for hiding this comment

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

I suspect we also have some residua in the test build infra, around files like CLRTest.Crossgen.targets, R2RTest and such. I'll float their cleanup as one of the ideas for the Quality week next week.

@MichalStrehovsky
Copy link
Member

Cc @dotnet/samsung crossgen1 is getting removed here, as indicated before - note this is targeting main branch (.NET 7). .NET 6 got snapped into the release/6.0 branch.

@davidwrighton
Copy link
Member

davidwrighton commented Aug 17, 2021

Could you also remove the references to FEATURE_PREJIT and FEATURE_NGEN_RELOCS_OPTIMIZATIONS from clrdefinitions.cmake? That code is also now completely and totally dead as of crossgen no longer being in our build.

@trylek
Copy link
Member

trylek commented Aug 17, 2021

I cannot resist sharing this small reminder regarding our recent journey. Thanks so much to everyone involved!

From: Zach Montoya zamont@microsoft.com
Sent: Tuesday, May 1, 2018 10:02 PM
To: Jan Kotas jkotas@microsoft.com
Cc: Simon Nattress simonn@microsoft.com; Tomas Rylek trylek@microsoft.com
Subject: Ready To Run Chat / Brain Dump

Hi Jan,

Would you mind taking an hour sometime this week to give me (and possibly Simon & Tomas if interested) a high-level talk on Ready to Run? I have no experience with Ready to Run at the moment so I’d love to get a brain dump from you so I have an improved understanding of it, especially since Sergiy’s new intern will be arriving on Monday and I’ll be coaching her through the internship.

If that’s fine with you, then Simon & Tomas let me know as well if you would be interested so we can coordinate a good meeting time.

Thanks,
Zach

Just for context, the intern, Amy Yu, ended up writing the first version of the R2RDump tool.

@jkoritzinsky
Copy link
Member Author

Test failures are known and linked. Only build failure is the feed instability. I'm going to merge.

@jkoritzinsky jkoritzinsky merged commit c07d830 into dotnet:main Aug 18, 2021
@jkoritzinsky jkoritzinsky deleted the remove-crossgen1-build-infra branch August 18, 2021 16:46
@jkoritzinsky
Copy link
Member Author

This removal cut an execution of ./src/coreclr/build-runtime.cmd from 5 minutes 30 seconds with all cores focusing on compile to 4 minutes 52 seconds while one core was being pegged by my antivirus (out of my control). So that's a reduction of about 7 CPU minutes if everything were on a single thread.

@am11
Copy link
Member

am11 commented Aug 18, 2021

Thanks! Great impactful cleanup indeed! 8-)

@ghost ghost locked as resolved and limited conversation to collaborators Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CrossGen/NGEN-coreclr tenet-build-performance Impacts build time: official, developer or CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants