Add test for profiler inlining #60225

davmason · 2021-10-10T01:05:50Z

We found out recently (#59741) that we have no coverage for whether the runtime respects the profiler's request to prevent or allow inlining. This adds some basic tests

ghost · 2021-10-10T01:05:56Z

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

Issue Details

We found out recently (#59741) that we have no coverage for whether the runtime respects the profiler's request to prevent or allow inlining. This adds some basic tests

Author:	davmason
Assignees:	davmason
Labels:	`area-Diagnostics-coreclr`
Milestone:	7.0.0

davmason · 2021-10-20T08:40:14Z

@noahfalk @josalem @hoyosjs does anybody have a minute to look at this?

tommcdon

LGTM, one minor comment for your review

tommcdon · 2021-10-20T10:06:11Z

src/tests/profiler/unittest/inlining.cs

+            Console.WriteLine($"Inlining, x={x}");
+        }
+
+        [MethodImpl(MethodImplOptions.NoInlining)]


Minor Nit: Do we mean to block inlining via attribute on this method? It seems we want to verify that the profiler is making the decision to block inlining for this function.

Looking at the test I think the only method where David intended to modify the inlining behavior from the profiler is "Inlinee". For all other methods we are just ensuring they don't unintentionally get inlined so that the test will get enter/exit callbacks for them.

Yeah, what Noah said 😄. I should have called this method "Inliner" and not "Inlining". I think it's actually worth updating for clarity

noahfalk

LGTM!
Are you able to run this test against any .NET 6 build without your recent fixes and confirm the test fails?

davmason · 2021-10-21T08:52:59Z

Are you able to run this test against any .NET 6 build without your recent fixes and confirm the test fails?

I can now confirm that it fails when targeting 6.0-rc1

BEGIN EXECUTION
 "C:\git\runtime\artifacts\tests\coreclr\windows.x64.Checked\Tests\Core_Root\\corerun.exe" -p "System.Reflection.Metadata.MetadataUpdater.IsSupported=false" inlining.dll
Profiler path: C:\git\runtime\artifacts\tests\coreclr\windows.x64.Checked\profiler\unittest\inlining\Profiler.dll
Disabling tiered compilation, jitstress, and minopts.
Profiler.dll!DllGetClassObject
Profiler.dll!Profiler::Initialize
Inlining, x=604021970
BlockInlining, x=159477398
Saw Inlinee as a real method call instead of inlined.
NoResponse, x=1597929155
Profiler.dll!Profiler::Shutdown
TEST FAILED _failures=1_sawInlineeCall=1

Test failed: Profiler tests are expected to contain the text 'PROFILER TEST PASSES' in the console output of the profilee app to indicate a passing test. Usually it is printed from the Shutdown() method of the profiler implementation. This text was not found in the output above.
Unhandled exception. System.Exception: Profiler tests are expected to contain the text 'PROFILER TEST PASSES' in the console output of the profilee app to indicate a passing test. Usually it is printed from the Shutdown() method of the profiler implementation. This text was not found in the output above.
   at Profiler.Tests.ProfilerTestRunner.FailFastWithMessage(String error) in C:\git\runtime\src\tests\profiler\common\ProfilerTestRunner.cs:line 190
   at Profiler.Tests.ProfilerTestRunner.Run(String profileePath, String testName, Guid profilerClsid, String profileeArguments, ProfileeOptions profileeOptions, Dictionary`2 envVars, String reverseServerName, Boolean loadAsNotification, Int32 notificationCopies)
   at Profiler.Tests.InliningTest.Main(String[] args) in C:\git\runtime\src\tests\profiler\unittest\inlining.cs:line 60
Expected: 100
Actual: -532462766

BruceForstall · 2021-10-21T18:04:32Z

@davmason I think this change is causing my PR to fail in the GCC build:

[ 87%] Linking CXX shared library libDelegateTestNative.so
/__w/1/s/src/tests/profiler/native/eltprofiler/slowpatheltprofiler.cpp:30:82: error: ‘visibility’ attribute ignored [-Werror=attributes]
   30 | PROFILER_STUB EnterStub(FunctionIDOrClientID functionId, COR_PRF_ELT_INFO eltInfo)
      |                                                                                  ^
/__w/1/s/src/tests/profiler/native/eltprofiler/slowpatheltprofiler.cpp:37:82: error: ‘visibility’ attribute ignored [-Werror=attributes]
   37 | PROFILER_STUB LeaveStub(FunctionIDOrClientID functionId, COR_PRF_ELT_INFO eltInfo)
      |                                                                                  ^
/__w/1/s/src/tests/profiler/native/eltprofiler/slowpatheltprofiler.cpp:44:85: error: ‘visibility’ attribute ignored [-Werror=attributes]
   44 | PROFILER_STUB TailcallStub(FunctionIDOrClientID functionId, COR_PRF_ELT_INFO eltInfo)
      |                                                                                     ^
[ 88%] Linking CXX shared library native_i7c/native_i7c.so
[ 88%] Built target DelegateTestNative

https://dev.azure.com/dnceng/public/_build/results?buildId=1434508&view=logs&jobId=43e4a348-e016-5d2a-a84b-7c597b487768&j=43e4a348-e016-5d2a-a84b-7c597b487768&t=0427443e-d386-5ea1-3848-e44c274b3f54

davmason · 2021-10-21T19:08:07Z

@BruceForstall interesting, do you know why that leg didn't run on this PR? It looks like it should be an easy fix, I'm taking a look

BruceForstall · 2021-10-21T19:50:41Z

Good question; I don't know.

I know @mangod9 (and I) encountered this problem recently. There was some issue with the version of gcc on some machines differing from others (?). @mangod9 Any info here?

am11 · 2021-10-21T20:30:44Z

do you know why that leg didn't run on this PR? It looks like it should be an easy fix, I'm taking a look

The GCC leg was skipped in this PR because it is currently only triggered when src/coreclr has changed. Since src/tests and src/native also takes part in what gcc leg builds, we should adjust this condition:

runtime/eng/pipelines/runtime.yml

Line 111 in 55984c8

    
                     eq(dependencies.evaluate_paths.outputs['SetPathVars_coreclr.containsChange'], true),

to include:

          eq(dependencies.evaluate_paths.outputs['SetPathVars_runtimetests.containsChange'], true),

am11 · 2021-10-21T20:40:19Z

https://dev.azure.com/dnceng/public/_build/results?buildId=1412218&view=logs&jobId=02b1a53f-5433-5236-da02-5fc377976c89&j=74429423-07be-5ff6-a774-8747bd8019a1

added trigger in #60738

mangod9 · 2021-10-21T20:51:30Z

The issue I ran into was where different CI machines had either gcc version 9 or 11. A particular codegen change was failing only on 11, so that can cause discrepancy in job failures. Dont think we looked into it further to determine why the versions are different.

am11 · 2021-10-21T20:53:45Z

It was because that other PR was sitting there for over a month and merged without re-running the CI, and during that time gcc leg was updated from v9 to v11.

add test for profiler inlining

d607f42

davmason added the area-Diagnostics-coreclr label Oct 10, 2021

davmason added this to the 7.0.0 milestone Oct 10, 2021

davmason requested a review from a team October 10, 2021 01:05

davmason self-assigned this Oct 10, 2021

tommcdon approved these changes Oct 20, 2021

View reviewed changes

noahfalk approved these changes Oct 20, 2021

View reviewed changes

davmason merged commit aea5cb9 into dotnet:main Oct 21, 2021

davmason deleted the inlining_test branch October 21, 2021 09:27

am11 mentioned this pull request Oct 21, 2021

Trigger GCC leg when src/tests is modified #60738

Closed

AndyAyersMS mentioned this pull request Oct 21, 2021

JIT: some enhancements to redundant branch opts #60732

Merged

ghost locked as resolved and limited conversation to collaborators Nov 21, 2021

Add test for profiler inlining #60225

Add test for profiler inlining #60225

Uh oh!

Conversation

davmason commented Oct 10, 2021

Uh oh!

ghost commented Oct 10, 2021

Uh oh!

davmason commented Oct 20, 2021

Uh oh!

tommcdon left a comment

Choose a reason for hiding this comment

Uh oh!

tommcdon Oct 20, 2021

Choose a reason for hiding this comment

Uh oh!

noahfalk Oct 20, 2021

Choose a reason for hiding this comment

Uh oh!

davmason Oct 20, 2021

Choose a reason for hiding this comment

Uh oh!

noahfalk left a comment

Choose a reason for hiding this comment

Uh oh!

davmason commented Oct 21, 2021

Uh oh!

BruceForstall commented Oct 21, 2021

Uh oh!

davmason commented Oct 21, 2021

Uh oh!

BruceForstall commented Oct 21, 2021

Uh oh!

am11 commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

am11 commented Oct 21, 2021

Uh oh!

mangod9 commented Oct 21, 2021

Uh oh!

am11 commented Oct 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

am11 commented Oct 21, 2021 •

edited

Loading