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.

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

Allow rejit on attach #19054

Merged
merged 20 commits into from Aug 1, 2018

Conversation

Projects
None yet
5 participants
@davmason
Member

davmason commented Jul 20, 2018

No description provided.

@davmason davmason self-assigned this Jul 20, 2018

@davmason davmason requested review from noahfalk and sywhang Jul 20, 2018

Show outdated Hide outdated src/vm/rejit.inl
Show outdated Hide outdated src/vm/rejit.inl
Show outdated Hide outdated src/inc/clrconfigvalues.h
@AndyAyersMS

This comment has been minimized.

Show comment
Hide comment
@AndyAyersMS

AndyAyersMS Jul 20, 2018

Member

Can you say more about the implications of this for codegen? In particular will we now see jump stamps by default for jitted code when tiering is disabled?

Member

AndyAyersMS commented Jul 20, 2018

Can you say more about the implications of this for codegen? In particular will we now see jump stamps by default for jitted code when tiering is disabled?

@davmason

This comment has been minimized.

Show comment
Hide comment
@davmason

davmason Jul 20, 2018

Member

@AndyAyersMS this will enable jump stamps by default, but since @noahfalk recently enabled tiered jitting by default my reasoning was this should have minimal impact. The only way you'll end up with tiered jitting disabled and rejit on is if the user explicitly opts out of tiered jitting.

We could have disabling tiered jitting also disable rejit on attach if this is a concern, I'm not sure if it's better to have the somewhat unexpected behavior that disabling tiered jitting diasbles rejit on attach too, or if it's better to have the somewhat unexpected behavior that disabling tiered jitting doesn't fully disable versioned code.

Member

davmason commented Jul 20, 2018

@AndyAyersMS this will enable jump stamps by default, but since @noahfalk recently enabled tiered jitting by default my reasoning was this should have minimal impact. The only way you'll end up with tiered jitting disabled and rejit on is if the user explicitly opts out of tiered jitting.

We could have disabling tiered jitting also disable rejit on attach if this is a concern, I'm not sure if it's better to have the somewhat unexpected behavior that disabling tiered jitting diasbles rejit on attach too, or if it's better to have the somewhat unexpected behavior that disabling tiered jitting doesn't fully disable versioned code.

@noahfalk

This comment has been minimized.

Show comment
Hide comment
@noahfalk

noahfalk Jul 20, 2018

Member

but since @noahfalk recently enabled tiered jitting by default

This hasn't occurred yet btw, but it will be coming soon. I don't think the exact timing matters for the outcome here but just wanted to be accurate.

Member

noahfalk commented Jul 20, 2018

but since @noahfalk recently enabled tiered jitting by default

This hasn't occurred yet btw, but it will be coming soon. I don't think the exact timing matters for the outcome here but just wanted to be accurate.

@AndyAyersMS

This comment has been minimized.

Show comment
Hide comment
@AndyAyersMS

AndyAyersMS Jul 20, 2018

Member

I don't know the right answer, just that jump stamps increase the size of the code for small methods and there are a lot of small methods. Would be nice to know roughly how much size increase we're talking about here...

FYI @dotnet/jit-contrib this would impact pmi diffs as we's see jump stamp padding in methods (like we do now some prejitted cases).

Member

AndyAyersMS commented Jul 20, 2018

I don't know the right answer, just that jump stamps increase the size of the code for small methods and there are a lot of small methods. Would be nice to know roughly how much size increase we're talking about here...

FYI @dotnet/jit-contrib this would impact pmi diffs as we's see jump stamp padding in methods (like we do now some prejitted cases).

@noahfalk

This comment has been minimized.

Show comment
Hide comment
@noahfalk

noahfalk Jul 20, 2018

Member

Obviously it will depend somewhat on the scenario. As one data point we enabled jump stamps for NGEN images by default years ago and my recollection is it cost us ~1-2% in image size.

Member

noahfalk commented Jul 20, 2018

Obviously it will depend somewhat on the scenario. As one data point we enabled jump stamps for NGEN images by default years ago and my recollection is it cost us ~1-2% in image size.

davmason added some commits Jul 24, 2018

Change places where the jit checks for rejit being enabled to actuall…
…y check for what it wants, which is whether jump stamps are enabled
@davmason

This comment has been minimized.

Show comment
Hide comment
@davmason

davmason Jul 24, 2018

Member

@noahfalk I should have all your feedback addressed here. I changed the couple places you mentioned to check IsVersionableWithJumpStamp and cached the value of CorProfilerEnableRejit

Member

davmason commented Jul 24, 2018

@noahfalk I should have all your feedback addressed here. I changed the couple places you mentioned to check IsVersionableWithJumpStamp and cached the value of CorProfilerEnableRejit

static bool profilerStartupRejit = CORProfilerEnableRejit() != FALSE;
static ConfigDWORD rejitOnAttachEnabled;
return profilerStartupRejit || (rejitOnAttachEnabled.val(CLRConfig::EXTERNAL_ProfAPI_RejitOnAttach) != 0);

This comment has been minimized.

@noahfalk

noahfalk Jul 24, 2018

Member

That got some of the cases, but there are other cases where the API result remains mutable. Imagine ProfAPI_RejitOnAttach = 0 and
a) a startup profiler enables rejit, then detaches. Previously this was disallowed because rejit was immutable and changing immutable flags stopped the profiler from detaching. I'd suggest you re-instate the restriction.
b) an attach profiler enables rejit. Similar to above this was disallowed because of the immutability rules but now it is allowed. I'd suggest you only allow attach profilers to set the bit if IsReJITEnabled is already true.

@noahfalk

noahfalk Jul 24, 2018

Member

That got some of the cases, but there are other cases where the API result remains mutable. Imagine ProfAPI_RejitOnAttach = 0 and
a) a startup profiler enables rejit, then detaches. Previously this was disallowed because rejit was immutable and changing immutable flags stopped the profiler from detaching. I'd suggest you re-instate the restriction.
b) an attach profiler enables rejit. Similar to above this was disallowed because of the immutability rules but now it is allowed. I'd suggest you only allow attach profilers to set the bit if IsReJITEnabled is already true.

This comment has been minimized.

@mjsabby

mjsabby Jul 25, 2018

Member

@noahfalk But will that mean that by default .NET Core apps will not get ReJit on Attach? That's a very useful thing to do.

@mjsabby

mjsabby Jul 25, 2018

Member

@noahfalk But will that mean that by default .NET Core apps will not get ReJit on Attach? That's a very useful thing to do.

This comment has been minimized.

@noahfalk

noahfalk Jul 25, 2018

Member

But will that mean that by default .NET Core apps will not get ReJit on Attach?

Nope : ) The default value of CLRConfig::EXTERNAL_ProfAPI_RejitOnAttach = 1 so by default this API returns true.

@noahfalk

noahfalk Jul 25, 2018

Member

But will that mean that by default .NET Core apps will not get ReJit on Attach?

Nope : ) The default value of CLRConfig::EXTERNAL_ProfAPI_RejitOnAttach = 1 so by default this API returns true.

This comment has been minimized.

@davmason

davmason Jul 25, 2018

Member

@noahfalk I thought for some reason there was already a check for enabling rejit separately from the immutable flags, but looking back over the code I don't know where I got that idea from.

I just added a commit to prevent rejit flags from being set unless it's either during startup or is already enabled, and to prevent detach if rejit is enabled. SetEventMask will return CORPROF_E_REJIT_NOT_ENABLED, that was the closest error message I could find. Should I add a more specific one since this one was originally intended to mean that you called a rejit api without enabling it, or is overloading the error message ok?

@davmason

davmason Jul 25, 2018

Member

@noahfalk I thought for some reason there was already a check for enabling rejit separately from the immutable flags, but looking back over the code I don't know where I got that idea from.

I just added a commit to prevent rejit flags from being set unless it's either during startup or is already enabled, and to prevent detach if rejit is enabled. SetEventMask will return CORPROF_E_REJIT_NOT_ENABLED, that was the closest error message I could find. Should I add a more specific one since this one was originally intended to mean that you called a rejit api without enabling it, or is overloading the error message ok?

This comment has been minimized.

@noahfalk

noahfalk Jul 25, 2018

Member

Seems ok to me

@noahfalk

noahfalk Jul 25, 2018

Member

Seems ok to me

disallow detach after setting rejit event mask, and prevent it from b…
…eing set if rejit on attach is turned off
@@ -2056,6 +2057,16 @@ HRESULT EEToProfInterfaceImpl::EnsureProfilerDetachable()
return CORPROF_E_IRREVERSIBLE_INSTRUMENTATION_PRESENT;
}
if (m_fModifiedRejitState)

This comment has been minimized.

@noahfalk

noahfalk Jul 25, 2018

Member

I think this check is identical to:

if(CORProfilerEnableRejit()) 

in which case the new modified flag isn't needed.

@noahfalk

noahfalk Jul 25, 2018

Member

I think this check is identical to:

if(CORProfilerEnableRejit()) 

in which case the new modified flag isn't needed.

This comment has been minimized.

@davmason

davmason Jul 26, 2018

Member

The way I have it written, I don't believe it is identical. In SetEventMask, I am not currently preventing profilers from disablingCOR_PRF_ENABLE_REJIT, so it would be possible for a profiler to detach after setting the event mask to not include COR_PRF_ENABLE_REJIT if there was only a check for CORProfilerEnableRejit().

So, either we need the check that I added to see if it's ever been enabled, or prevent SetEventMask from turning off COR_PRF_ENABLE_REJIT once it's been enabled. I'm not particularly partial to either one, this is just the one I chose.

@davmason

davmason Jul 26, 2018

Member

The way I have it written, I don't believe it is identical. In SetEventMask, I am not currently preventing profilers from disablingCOR_PRF_ENABLE_REJIT, so it would be possible for a profiler to detach after setting the event mask to not include COR_PRF_ENABLE_REJIT if there was only a check for CORProfilerEnableRejit().

So, either we need the check that I added to see if it's ever been enabled, or prevent SetEventMask from turning off COR_PRF_ENABLE_REJIT once it's been enabled. I'm not particularly partial to either one, this is just the one I chose.

This comment has been minimized.

@noahfalk

noahfalk Jul 26, 2018

Member

You are absolutely right, lets leave like this.

@noahfalk

noahfalk Jul 26, 2018

Member

You are absolutely right, lets leave like this.

static bool profilerStartupRejit = CORProfilerEnableRejit() != FALSE;
static ConfigDWORD rejitOnAttachEnabled;
return profilerStartupRejit || (rejitOnAttachEnabled.val(CLRConfig::EXTERNAL_ProfAPI_RejitOnAttach) != 0);

This comment has been minimized.

@noahfalk

noahfalk Jul 25, 2018

Member

Seems ok to me

@noahfalk

noahfalk Jul 25, 2018

Member

Seems ok to me

@davmason

This comment has been minimized.

Show comment
Hide comment
@davmason

davmason Jul 31, 2018

Member

@dotnet-bot test Ubuntu x64 Checked CoreFX Tests

Member

davmason commented Jul 31, 2018

@dotnet-bot test Ubuntu x64 Checked CoreFX Tests

@davmason

This comment has been minimized.

Show comment
Hide comment
@davmason

davmason Aug 1, 2018

Member

@noahfalk I found two issues while running diagnostics tests. One was the HasDefaultIL issue we talked about, the other is I wasn't taking the CodeManagerLock in SetIP. They're small, but I'm letting you know in case you want to take a look

Member

davmason commented Aug 1, 2018

@noahfalk I found two issues while running diagnostics tests. One was the HasDefaultIL issue we talked about, the other is I wasn't taking the CodeManagerLock in SetIP. They're small, but I'm letting you know in case you want to take a look

@noahfalk

This comment has been minimized.

Show comment
Hide comment
@noahfalk

noahfalk Aug 1, 2018

Member

seems fine to me, thanks for the heads up : )

Member

noahfalk commented Aug 1, 2018

seems fine to me, thanks for the heads up : )

@davmason davmason merged commit 1487fa7 into dotnet:master Aug 1, 2018

27 checks passed

CROSS Check Build finished.
Details
CentOS7.1 x64 Checked Innerloop Build and Test Build finished.
Details
CentOS7.1 x64 Debug Innerloop Build Build finished.
Details
Linux-musl x64 Debug Build Build finished.
Details
OSX10.12 x64 Checked CoreFX Tests Build finished.
Details
OSX10.12 x64 Checked Innerloop Build and Test Build finished.
Details
Tizen armel Cross Checked Innerloop Build and Test Build finished.
Details
Ubuntu arm Cross Checked Innerloop Build and Test Build finished.
Details
Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test Build finished.
Details
Ubuntu arm64 Cross Debug Innerloop Build Build finished.
Details
Ubuntu x64 Checked CoreFX Tests Build finished.
Details
Ubuntu x64 Checked Innerloop Build and Test Build finished.
Details
Ubuntu x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0) Build finished.
Details
Ubuntu x64 Formatting Build finished.
Details
WIP ready for review
Details
Windows_NT x64 Checked CoreFX Tests Build finished.
Details
Windows_NT x64 Checked Innerloop Build and Test Build finished.
Details
Windows_NT x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0) Build finished.
Details
Windows_NT x64 Formatting Build finished.
Details
Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x64 min_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 Checked Innerloop Build and Test Build finished.
Details
Windows_NT x86 Checked Innerloop Build and Test (Jit - TieredCompilation=0) Build finished.
Details
Windows_NT x86 Release Innerloop Build and Test Build finished.
Details
Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 min_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
license/cla All CLA requirements met.
Details

@davmason davmason deleted the davmason:rejit_attach branch Aug 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment