Skip to content

Conversation

kg
Copy link
Member

@kg kg commented Jul 18, 2025

  • Include the name of the current method in the replacement strings inserted by the custom profiler
  • Capture and validate the stdout messages in the test so we can check whether the rejit revert actually worked
  • Fix failure when running r2r composite version of test on some configurations (it expected inlining to happen, but there's no guarantee it will)
  • Fix a small oversight in rejit.cpp

@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 18, 2025
kg added 2 commits July 18, 2025 10:27
…de the method name in the replacement string so it is easier to tell what is going on

Capture and validate actual output in the profiler rejit test
@kg kg force-pushed the rejit-profiler-test-fixes branch from a6f60f7 to 99ac4b3 Compare July 21, 2025 17:05
@kg
Copy link
Member Author

kg commented Jul 21, 2025

/azp run runtime-coreclr crossgen2 outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kg
Copy link
Member Author

kg commented Jul 21, 2025

/azp run runtime-coreclr crossgen2 outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kg
Copy link
Member Author

kg commented Jul 22, 2025

OK, reverts being broken aside I think what's going on is that on the composite R2R side, rejitting the method sometimes doesn't rejit any of its callers, which means no inlining happens and the profiler test breaks (it expects to see inlining). I think the R2R composite behavior is correct based on the output, and revert actually works correctly:

Profilee STDOUT: TriggerDirectInlining
Profilee STDOUT: Hello from profiler rejit method 'InlineeTarget'!  (TriggerDirectInlining)
Profilee STDOUT: CallMethodWithoutInlining
Profilee STDOUT: Hello from profiler rejit method 'InlineeTarget'!  (CallMethodWithoutInlining)
Profilee STDOUT: TriggerInliningChain
Profilee STDOUT:  Inline.InlineeChain1
Profilee STDOUT: Hello from profiler rejit method 'InlineeTarget'!  (InlineeChain1)

Profilee STDOUT: Revert should be triggered after this method...
Profilee STDOUT: TriggerDirectInlining
Profilee STDOUT:   Inline.InlineeTarget (TriggerDirectInlining)
Profilee STDOUT: CallMethodWithoutInlining
Profilee STDOUT:   Inline.InlineeTarget (CallMethodWithoutInlining)
Profilee STDOUT: TriggerInliningChain
Profilee STDOUT:  Inline.InlineeChain1
Profilee STDOUT:   Inline.InlineeTarget (InlineeChain1)

@kg kg changed the title Improvements to rejit profiler test Fix issues with rejit profiler test Jul 22, 2025
@kg
Copy link
Member Author

kg commented Jul 22, 2025

@noahfalk if you get a chance, can you tell me whether the rejit.cpp change is correct? It looked like currently we might erroneously skip doing a revert.

@kg
Copy link
Member Author

kg commented Jul 22, 2025

/azp run runtime-coreclr crossgen2 outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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.

Thanks @kg!

@kg kg marked this pull request as ready for review July 22, 2025 23:21
@Copilot Copilot AI review requested due to automatic review settings July 22, 2025 23:21
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes several issues with the rejit profiler test to make it more reliable and properly validate rejit functionality. The changes address test flakiness in R2R composite configurations and improve validation by capturing stdout output.

  • Enhances test validation by capturing and checking stdout messages to verify rejit and revert operations
  • Improves profiler reliability by filtering out non-test module inlinings and including method names in replacement strings
  • Fixes a parameter mismatch in the CoreCLR rejit implementation and temporarily re-enables the test

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/tests/profiler/rejit/rejit.cs Adds output capturing mechanism and validation logic for rejit/revert operations
src/tests/profiler/native/rejitprofiler/rejitprofiler.cpp Includes method names in profiler messages and filters inlinings to test module only
src/tests/issues.targets Temporarily re-enables the rejit test for validation
src/coreclr/vm/rejit.h Removes unused fIsRevert parameter from method signature
src/coreclr/vm/rejit.cpp Fixes parameter passing by removing unused fIsRevert parameter

@kg kg merged commit b5d37d1 into dotnet:main Jul 23, 2025
101 of 103 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 23, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants