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.

Sign up for GitHub

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

Jump to bottom

Implement JitInfo API #55046

Merged
merged 22 commits into from
Jul 14, 2021
Merged

Implement JitInfo API #55046

merged 22 commits into from
Jul 14, 2021

Conversation

josalem
Copy link
Contributor

@josalem josalem commented Jul 1, 2021 •
edited
Loading

Implements #54444

This PR adds the System.Runtime.JitInfo static class and static methods. These are used for collecting information about the Just In Time compiler. As implemented, the API allows you retrieve:

  • The number of bytes of IL compiled
  • The number of methods compiled
  • The time spent compiling method

Some side-effects to this patch:

  • Adds a NormalizedTimer class to the CoreCLR utils for collecting time differences in 100ns ticks (the same ticks used by TimeSpan.FromTicks()). This should help avoid discrepancies in tick definitions between managed and native code.
  • Updates runtime counters to use new API
  • Updates eventpipe internals code for mono to include a icall that collects jit time values

CC @tommcdon @benmwatson @lateralusX

John Salem added 3 commits June 4, 2021 18:57
Add JIT time metric
264a74f 
* add per thread privately
* add test for counter
* add test for priuvate metric (currently failing due to linker I think)
implement coreclr side
8c2ef38
implement mono side
e63ed01
@josalem josalem added this to the 6.0.0 milestone Jul 1, 2021
@josalem josalem self-assigned this Jul 1, 2021
@ghost
Copy link

ghost commented Jul 1, 2021

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

Implements #54444

TODO:

  • disable API for browser/wasm
  • update test to handle 0 on Mono for currentThread = true
  • ensure /// docs are sufficient

Putting this up in draft form for some early feedback.

CC @tommcdon @benmwatson @lateralusX

Author: josalem
Assignees: josalem
Labels:

area-System.Runtime
Milestone: 6.0.0

Feedback and build fix
0e16d4a 
* Fix mono build by adding mono partial class to csproj
* PR feedback
  * no interlocked* for thread_local
  * use CLR_BOOL
  * use LARGE_INTEGER and only static_cast once
* correct ticks calculation
  * convert Stopwatch methods to internal
  * use same conversion as S.D.Stopwatch
PR feedback + adding ref source
064fd0c 
* normalize ticks to 100ns in native code
  * add NormalizedTimer class
* added source ref
* change ret value to long
jkotas
jkotas reviewed Jul 2, 2021
View reviewed changes
src/coreclr/vm/util.hpp Outdated Show resolved Hide resolved
John Salem added 5 commits July 2, 2021 17:29
* use constructor instead of intiializer
780d8a9 
* use double for frequency in case
  clock resolution is larger than 100ns
fix helper function visibility
1471a64
Fix test failure
0dd2832 
* test was attempting to parse ints when the interval
 can be a double
Improve test:
a7aa30a 
* skip on aot platforms
* add current thread test for coreclr
Further test improvements
5df31df
@josalem josalem requested review from lateralusX and a team July 7, 2021 23:43
noahfalk
noahfalk approved these changes Jul 8, 2021
View reviewed changes
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.

Made a few suggestions on the tests, but overall lgtm!
I take it you have looked at some generated numbers manually to confirm the results look reasonable?

src/coreclr/vm/util.hpp Outdated Show resolved Hide resolved
src/libraries/System.Runtime/tests/System/Runtime/JitInfoTests.cs Outdated Show resolved Hide resolved
src/libraries/System.Runtime/tests/System/Runtime/JitInfoTests.cs Show resolved Hide resolved
src/libraries/System.Runtime/tests/System/Runtime/JitInfoTests.cs Outdated Show resolved Hide resolved
src/libraries/System.Runtime/tests/System/Runtime/JitInfoTests.cs Outdated Show resolved Hide resolved
John Salem added 5 commits July 8, 2021 10:42
PR feedback
dcfdfb7 
* Removed Volatile<T> from globals
  since they are all accessed either via InterlockedExchange
  or VolatileLoad
* Added no tearing helper function for 32 bit systems
* updated tests to compare before and after
* added tests for AOT and Mono specific behaviors
* furether simplified NormalizedTimer class with asserts
Fix typo in mono icalls
61f90db
fix typo
39e8859
remove unneeded cast
3546750
Force interpretation of T as int64_t on x86
108d64d
@josalem
Copy link
Contributor Author

josalem commented Jul 8, 2021

Does anyone know a way to detect whether a test is being run under the Mono interpreter? If not, I'm going to modify the Mono flavor of the test to accept 0 and (after-before)==0.

@jkotas
Copy link
Member

jkotas commented Jul 8, 2021

PlatformDetection.IsMonoInterpreter ?

Update tests based on PR feedback
45d846b 
* use ref emit dynamic method
* adds IsNotMonoAot platform detection
@josalem
Copy link
Contributor Author

josalem commented Jul 12, 2021

Barring any CI failures, I'm hoping to merge this later today.

@runfoapp runfoapp bot mentioned this pull request Jul 13, 2021
Closed
@josalem
Copy link
Contributor Author

josalem commented Jul 13, 2021

Remaining failure seems to be #55536.

@josalem
Copy link
Contributor Author

josalem commented Jul 13, 2021

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@josalem
Copy link
Contributor Author

josalem commented Jul 14, 2021

I have a successful CI run; I'm intending to merge this PR this morning.

@josalem josalem merged commit 21a7632 into dotnet:main Jul 14, 2021
@josalem josalem deleted the dev/josalem/per-thread-jit-metrics branch July 14, 2021 17:47
@josalem josalem mentioned this pull request Jul 15, 2021
Closed
@ManickaP ManickaP mentioned this pull request Jul 20, 2021
Merged
This was referenced Jul 21, 2021
Closed
Closed
Closed
@karelz karelz mentioned this pull request Jul 28, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@reflectronic reflectronic reflectronic left review comments

@tannergooding tannergooding tannergooding left review comments

@jkotas jkotas jkotas left review comments

@stephentoub stephentoub stephentoub left review comments

@lambdageek lambdageek lambdageek left review comments

@vargaz vargaz vargaz left review comments

@noahfalk noahfalk noahfalk approved these changes

@imhameed imhameed Awaiting requested review from imhameed

@marek-safar marek-safar Awaiting requested review from marek-safar

@SamMonoRT SamMonoRT Awaiting requested review from SamMonoRT

@thaystg thaystg Awaiting requested review from thaystg thaystg is a code owner

@lateralusX lateralusX Awaiting requested review from lateralusX

Assignees

@josalem josalem

Labels
area-System.Runtime
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
8 participants
@josalem @jkotas @vargaz @lambdageek @stephentoub @noahfalk @tannergooding @reflectronic