Skip to content

Conversation

simonferquel
Copy link
Contributor

@simonferquel simonferquel commented Mar 26, 2025

Implement the changes discussed in #113921, making COR_PRF_DISABLE_OPTIMIZATIONS and COR_PRF_DISABLE_INLINING dynamically flippable, and applied consistently per module.

@ghost ghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 26, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 26, 2025
@jkotas jkotas added area-Diagnostics-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Mar 26, 2025
Copy link
Contributor

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

Looks good so far. I like your suggestion to refactor CORDisableJITOptimizations as a Module helper function so I expect that will change it a bit.

@simonferquel
Copy link
Contributor Author

Made the suggested changes + tentatively fix test by filtering Inlining requests per module name)

@simonferquel
Copy link
Contributor Author

@tommcdon
Copy link
Member

tommcdon commented Apr 1, 2025

@simonferquel
Copy link
Contributor Author

I think the other failures are unrelated known instabilities

@simonferquel
Copy link
Contributor Author

Per review comments, did:

  • remove debug bits macro, encapsulating it in the helper method (actually enforcing both debugger and profiler bits are checked)
  • moved IsInliningDisabledByProfiler check within PROFILING_SUPPORTED and returning the same INLINING_FAIL result as initially
  • removed some redundant #ifdefs
  • reviewed both C# and C++ formatting (it does not help that all clang-format / clang-tidy rules are removed at the root of the repo)

Copy link
Member

@mdh1418 mdh1418 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 for your effort! I think it looks good as long as we can confirm the test passes in CI

@mdh1418 mdh1418 self-requested a review April 2, 2025 17:12
@simonferquel simonferquel force-pushed the make-cor-prf-disable-optimizations-non-immutable-upstream branch from c73170f to 954ebc3 Compare April 3, 2025 08:36
@simonferquel
Copy link
Contributor Author

Last round of update:

  • restored CORDebuggerAllowJITOpts macro, with a comment about preferring to call Module::AreJITOptimizationsDisabled()
  • reorganized commit history (it was starting to be a mess)
  • rebased to tentatively get any potential fixes for the seemingly unrelated build/test failures

@simonferquel
Copy link
Contributor Author

Rebase brought new build failures - but it seems they are their on tip of main branch as well

Copy link
Member

@noahfalk noahfalk 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 @simonferquel!

@simonferquel
Copy link
Contributor Author

Should I worry about the build / test failures ? they all seem unrelated but could use confirmation

@mdh1418
Copy link
Member

mdh1418 commented Apr 3, 2025

The Build Analysis pipeline should be green before we merge. It uses known issues to filter out failing checks, so the 3 other pipelines that are failing Build Libraries Test Run checked coreclr linux x64 Release, Build osx-x64 Release AllSubsets Mono Minijit RuntimeTests minijit, and Build osx-x64 Release NativeAOT have failures that either are

  1. directly caused by this PR - Should fix
  2. not related to this PR and there is no known issue linked to the failure - Should create a new issue to regex match the failure
  3. not related to this PR and there is a known issue linked to the failure, but the regex matching couldn't match it - should either fix the regex matching or mention in the issue that the failure also occured in this PR

Build Analysis Overview

Copy link
Member

@mdh1418 mdh1418 left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@mdh1418
Copy link
Member

mdh1418 commented Apr 3, 2025

All the Build Libraries Test Run checked coreclr linux x64 Release are known issues #114152 #113883 and #107761

@mdh1418
Copy link
Member

mdh1418 commented Apr 3, 2025

I think the OSX failures are some bad machine configuration.

@simonferquel, in addition to Jan's feedback, there are also conflicts in src/tests/profiler/native/CMakeLists.txt and src/tests/profiler/native/classfactory.cpp because
#114107 also added a profiler test

@simonferquel simonferquel force-pushed the make-cor-prf-disable-optimizations-non-immutable-upstream branch from b170069 to 2907634 Compare April 4, 2025 07:55
@simonferquel
Copy link
Contributor Author

I rebased and fixed conflict. I also opened a dummy draft PR (removing an empty line of code) to compare build results (so that I do not introduce regression by feeling a flakky test issue)

@simonferquel
Copy link
Contributor Author

Build analysis is green, thanks for the help working on that one :)

@mdh1418 mdh1418 merged commit f9cf3c8 into dotnet:main Apr 4, 2025
99 of 105 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators May 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Diagnostics-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants