Skip to content

Make COR_PRF_DISABLE_OPTIMIZATIONS Allowable After Attach and non immutable#113924

Merged
mdh1418 merged 5 commits intodotnet:mainfrom
simonferquel:make-cor-prf-disable-optimizations-non-immutable-upstream
Apr 4, 2025
Merged

Make COR_PRF_DISABLE_OPTIMIZATIONS Allowable After Attach and non immutable#113924
mdh1418 merged 5 commits intodotnet:mainfrom
simonferquel:make-cor-prf-disable-optimizations-non-immutable-upstream

Conversation

@simonferquel
Copy link
Copy Markdown
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
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@simonferquel
Copy link
Copy Markdown
Contributor Author

@tommcdon
Copy link
Copy Markdown
Member

tommcdon commented Apr 1, 2025

@simonferquel
Copy link
Copy Markdown
Contributor Author

I think the other failures are unrelated known instabilities

@simonferquel
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@mdh1418
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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