Simplified MinOpts GC ref tracking.#9231
Conversation
|
@dotnet-bot test CentOS7.1 gcstress0xc_minopts_heapverify1 |
|
test Windows_NT perf |
|
test Ubuntu14.04 perf |
|
@dotnet-bot test OSX Checked r2r_jitminopts |
|
@dotnet-bot test Ubuntu Checked r2r_jitminopts |
|
@dotnet-bot test Ubuntu Checked r2r_jitminopts |
|
@dotnet-bot test OSX gcstress0x3 |
|
@dotnet-bot test OSX Checked r2r_jitminopts |
|
@dotnet-bot test Ubuntu minopts |
|
@dotnet-bot test Ubuntu gcstress0xc_minopts_heapverify1 |
|
@dotnet-bot test CentOS7.1 gcstress0xc |
|
@dotnet-bot test Ubuntu minopts |
|
@swaroop-sridhar @jkotas @BruceForstall @sivarv PTAL |
|
@pkukol Looks like there are a bunch of test failures. Do you still want it reviewed now? |
|
@BruceForstall Yes please, most of the test failures appear to be spurious or are errors in the tests themselves; I am tracking two strange failures on desktop (one is COBOL and the other one is a complex physics simulation so it's proving to be a bit difficult) but other than that everything passes. Except for some flavors of Linux but I can't tell whether those are genuine failures or not. Still hoping to hear from someone as far as how to run tests on Linux with my build to close that loop. Sorry about that. |
|
@dotnet-bot test Ubuntu minopts |
|
@dotnet-bot test OSX minopts |
|
@dotnet-bot test OSX gcstress0x3 |
|
@dotnet-bot test CentOS7.1 gcstress0xc_minopts_heapverify1 |
|
@dotnet-bot test Ubuntu gcstress0x3 |
| GC.Collect(); | ||
| GC.WaitForPendingFinalizers(); | ||
| GC.Collect(); | ||
| for (int i = 0; i < 50; i++) |
There was a problem hiding this comment.
Would it make more sense to create a new DoCollect() function that does these three lines, is marked NoInlining, and is called between the Test1()/Test2()/etc. calls in Main()?
There was a problem hiding this comment.
This will be in the next push.
| GCENCODER_WITH_LOGGING(gcInfoEncoderWithLog, gcInfoEncoder); | ||
|
|
||
| #if defined(_TARGET_AMD64_) && defined(FEATURE_CORECLR) | ||
| const bool fastPath = (compiler->opts.MinOpts() && !compiler->opts.jitFlags->IsSet(JitFlags::JIT_FLAG_PREJIT) && |
There was a problem hiding this comment.
fastPath [](start = 15, length = 8)
fastPath seems too generic a name. Why not trackGcRefs or MinOptsTrackGcRefs?
There was a problem hiding this comment.
It's actually the opposite so I picked *"noTrackedGCSlots - hope that's OK.
| { | ||
| gcInfoEncoderWithLog->DefineCallSites(nullptr, nullptr, 0); | ||
| return; | ||
| } |
There was a problem hiding this comment.
It's unfortunate and undesirable to add a "return" in the middle of this enormous method. Isn't there a way to restructure the code to avoid this, and let it fall through (without adding too much complexity in the way of additional nesting, etc.)?
There was a problem hiding this comment.
The main goal is to skip all the code below here because we know (from the first pass) that we'll find no calls that we'll need to record, but there are usually quite a few calls on the list (they just all get skipped twice in 99.9% of the cases). The only way to avoid wrapping everything below in yet another level of nesting would be to bash the call list to null but that seems extremely dirty (and may not work). If you have a specific idea, though, please let me know - I agree the method is pretty knarly as it is.
| #endif // !defined(DEBUG) && !defined(_DEBUG) | ||
|
|
||
| #if defined(_TARGET_AMD64_) && defined(FEATURE_CORECLR) | ||
| CONFIG_INTEGER(JitMinOptsTrackGCrefs, W("JitMinOptsTrackGCrefs"), 0) // Allow GC roots to be tracked w/MinOpts |
There was a problem hiding this comment.
Why is this feature for AMD64 only? And why only CoreCLR? You can set the defaults for that, if you are worried about changing existing behavior. So, shouldn't it be:
#if !defined(JIT32_GCENCODER)
#if defined(_TARGET_AMD64_) && defined(FEATURE_CORECLR)
#define JitMinOptsTrackGCrefs_Default 0 // Not tracking GC refs in MinOpts is new behavior
#else
#define JitMinOptsTrackGCrefs_Default 1
#endif
CONFIG_INTEGER(JitMinOptsTrackGCrefs, W("JitMinOptsTrackGCrefs"), JitMinOptsTrackGCrefs_Default) // If non-zero, allow GC roots to be tracked w/MinOpts
#endif // !defined(JIT32_GCENCODER)
There was a problem hiding this comment.
Hmm, hold on - are you suggesting we ditch all the #ifdef's as well? That's not as obvious because even if it's normally disabled, it adds a bunch of code (mostly only 'if' statements but still) but more importantly in the GC decoder it weakens an assert that I think is quite important (I wanted to add an explicit bit, so that we don't have to assume that we see the new behavior by the absence of some stuff we normally require to be present, but there's no obvious place for a new flag, plus nobody else though it was worth a flag anyway). The suggestion to keep it out of the non-CORECLR build came from the VM folks so how about I leave it the way it is, since it's only been tested that way, and once it's proven its worth (which has yet to be verified independently) it can be enabled in more places?
There was a problem hiding this comment.
Yes, I was suggesting eliminating all the #ifdefs, so it would be possible to enable and test on desktop, or on non-AMD64 platforms, simply by setting a COMPlus variable. We have very few #ifdef FEATURE_CORECLR and I'd like to keep it that way.
There was a problem hiding this comment.
Missed your second comment until now; sorry about that. OK, I will change this as you suggest, but it still leaves the issue of the assert in vm\gcinfodecode.cpp - assuming it can see the above flag (the whole CLR vs. JIT config flags thing is still a mystery to me), it will need to test the flag when decoding GC info (at runtime) or we just weaken the decoding assert for every config / all the time? Neither approach seems ideal to me, but there's probably a way to do this better that I'm not seeing; please bear with me.
And, just to make absolutely sure - with this newest change, the new code will always be compiled in and we'll use a runtime test of (pseudo-code) config.JitMinOptsTrackGCrefs in the various spots in the *.cpp files instead of switching the code out completely (using the current "#ifdef AMD64 && blah blah"); correct?
There was a problem hiding this comment.
clrconfigvalues.h is for things read by the VM; jitconfigvalues.h is for things read by the VM. Note that clrconfigvalues.h does duplicate many things, if those things are used by JIT32/JIT64, which does not have jitconfigvalues.h and that mechanism. That is usually what causes confusion.
Yes, there will be code dynamically executed because of this, but presumably it will be very small/cheap.
| // No ranges and no explicit safepoint - must be MinOpts. | ||
| noTrackedRefs = true; | ||
| goto NO_RANGES; | ||
| } |
There was a problem hiding this comment.
Can you please figure out a way to avoid adding this goto?
There was a problem hiding this comment.
That's trivial to do (and I've tried it both ways) but when I asked earlier no feedback resulted so I left it alone; will do it by adding an 'if', then.
|
|
||
| /* static */ | ||
| int __cdecl Compiler::WtdRefCntCmpSimpleGC(const void* op1, const void* op2) | ||
| { |
There was a problem hiding this comment.
Please add a new standard-format function header.
You don't care about weighted ref counts in this case, right?, so why is it named "Wtd..."?
In fact, couldn't you assert:
assert(!dsc1->lvTracked);
assert(!dsc2->lvTracked);
?
So the only thing you are doing is sorting GC refs together?
There was a problem hiding this comment.
Decided to ditch this thing, it's just too awkward and ugly (and a maintenance danger); thanks for flagging it.
| } | ||
|
|
||
| // TODO(REVIEW): if we're here it means we have MinOpts and "simplified | ||
| // GC tracking", so we may be able to prune the stuff that follows. |
There was a problem hiding this comment.
Seems correct to me, if you add the assert I suggest.
There was a problem hiding this comment.
It seems like you should secondarily sort on weights, after GC-ness, before falling back to lclvar number to break ties.
|
@swaroop-sridhar @jkotas @BruceForstall @sivarv @pgavlin PTanotherL |
| else if (opts.MinOpts() && !JitConfig.JitMinOptsTrackGCrefs()) | ||
| { | ||
| if (varTypeIsGC(varDsc->TypeGet())) | ||
| { |
There was a problem hiding this comment.
minor: you may want all the conditions on the "else if" line above. Otherwise, if someone adds yet another "else if" case below, then it wouldn't get hit for the minopts & !JitMinOptsTrackGCrefs() case if the var isn't GC.
There was a problem hiding this comment.
Good point - in my testing version there's actually another statement in there so the two if's need to be separate but in the "real" version it's simple and might as well combine the conditions. Good catch, will include it in my next push.
|
@dotnet-bot test OSX minopts |
|
@dotnet-bot test Ubuntu minopts |
2 similar comments
|
@dotnet-bot test Ubuntu minopts |
|
@dotnet-bot test Ubuntu minopts |
|
@dotnet-bot test OSX minopts |
|
@dotnet-bot test Ubuntu minopts |
|
@dotnet-bot test Ubuntu minopts |
|
LGTM. Please squash commits before merging. |
|
@dotnet-bot test CentOS7.1 minopts |
|
@dotnet-bot test OSX Checked r2r_jitminopts |
Simplified MinOpts GC ref tracking - when a method is compiled with MinOpts for targets with the 64-bit GC info encoder we mark all GC slots as untracked and we omit encoding call sites with no live tracked GC refs explicitly in the GC tables; this can be controlled using the new COMPlus_JitMinOptsTrackGCrefs environment flag.
Simplified MinOpts GC ref tracking - when a method is compiled with MinOpts for targets with the 64-bit GC info encoder we mark all GC slots as untracked and we omit encoding call sites with no live tracked GC refs explicitly in the GC tables; this can be controlled using the new COMPlus_JitMinOptsTrackGCrefs environment flag. Commit migrated from dotnet/coreclr@d7509df
5 participants
Not ready for "official" review yet, but feel free to look and comment anytime ;).