Compile System.Private.CoreLib with Crossgen2 by default, take 2 #46051
Conversation
|
Interesting that the failure doesnt repro anymore. Could you please link the PRs for the fixes to this PR if they are available? |
|
Thanks for adding those, so now the only criteria is to have CI pass outerloop jobs for x86? |
|
Hmm, the non-deterministic nature of the bug makes it hard to define a precise criterion. On top of that, based on looking at the legs, I suspect that outerloop testing actually adds very little to libraries testing I'm most interested in at this moment. I guess that I'll just rerun the PR something like 5~10 times and I'll keep my fingers crossed - merging this in the next couple of days has the "thanksgiving advantage" that any irregularities will be easily seen in the reduced traffic of the holiday period. Having said that, the current working theory is that CG2 wasn't at fault at all by itself, it's just that some of the subtle diffs in code placement and import cell management aggravated the pre-existing GC hole and tailcall optimization bug that Bruce and JanK subsequently fixed. |
After Bruce's and JanK's fixes I'm no longer able to repro the previously observed failures in libraries tests locally so I'm switching back to lab testing; I plan to run several rounds of PR and outerloop tests to increase my confidence in the change. Thanks Tomas
trylek
force-pushed
the
SPCGC2_2ndAttempt
branch
from
a40ab79
to
7cf2252
Compare
| @@ -448,20 +448,7 @@ private Object HandleToObject(IntPtr handle) | |||
|
|
|||
| private bool Get_CORINFO_METHOD_INFO(MethodDesc method, MethodIL methodIL, CORINFO_METHOD_INFO* methodInfo) | |||
| { | |||
There was a problem hiding this comment.
Both crossgen1 and JIT are bailing out for PInvokes from getMethodInfo as well here:
runtime/src/coreclr/vm/jitinterface.cpp
Line 7783 in 891e8d4
Why is this change necessary?
There was a problem hiding this comment.
This change does not appear to be fixing the x86 crash based on my quick local test. Were you able to confirm locally that it is fixing the problem?
There was a problem hiding this comment.
While I believe I see a CG1 vs. CG2 diff in NgenDump I tracked down to this difference, I have rolled back the 2nd commit based on your clarification that the error I've been attempting to fix is unrelated to my change. Amusingly this is about the 2nd or 3rd time the SPC CG2 switchover revealed a bug that ended up being completely unrelated to the switchover as such. Let's hope it just means that we do take CG2 testing seriously ;-).
There was a problem hiding this comment.
While I believe I see a CG1 vs. CG2 diff in NgenDump I tracked down to this difference,
Do you still see the diff in the current master? If yes, could you please share the details of the diff?
trylek
force-pushed
the
SPCGC2_2ndAttempt
branch
from
7cf2252
to
61243bb
Compare
trylek
changed the title
|
I have removed the WIP status as the only remaining bug revealed by repeated testing of this change turns out to be unrelated to it - it's apparently #46184 based on JanK's clarification. I'll run another round of outerloop testing but right now I'm not aware of any remaining issues actually caused by this switch-over so I believe it's ready to be merged in. |
|
Superseded by #47019, closing. |
ghost
locked as resolved and limited conversation to collaborators
After Bruce's and JanK's fixes I'm no longer able to repro the
previously observed x86 failures in libraries tests locally so I'm
switching back to lab testing; I plan to run several rounds of
PR and outerloop tests to increase my confidence in the change.
Thanks
Tomas
/cc @dotnet/crossgen-contrib