Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Streamlined MinOpts GC ref tracking. #9869

Merged
merged 1 commit into from
Mar 1, 2017
Merged

Streamlined MinOpts GC ref tracking. #9869

merged 1 commit into from
Mar 1, 2017

Conversation

pkukol
Copy link

@pkukol pkukol commented Mar 1, 2017

When MinOpts is on for a method (and the COMPlus_JitMinOptsTrackGCrefs override is not on), 64-bit GC encoders will generate an abbreviated call site table and mark all GC ref stack slots as 'untracked'. The main purpose is to speed up compilation but this change should also shrink the GC tables (with MinOpts) by an average of about 2x.

@@ -628,6 +628,8 @@ bool GcInfoDecoder::EnumerateLiveSlots(


#ifdef PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED
bool noTrackedRefs = false;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CodegenMirror branch only mirrors the JIT directory not the VM (or inc) directories.
So the VM changes don't come over and won't automatically be part of any new Desktop releases.

If the VM changes rae required for correctness then they should be checked in to the CodegenMirror branch shortly after your PR goes through the mirror.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the heads up, @briansull - hopefully we'll be able to complete this thing quick this time around (it's almost exactly the same as last time, with one simplification that also fixes a weird problem that cropped up in one CoreFX test, BTW - @BruceForstall PTAL when you get a chance) and then I'll jump on the CGmirror. By default it should stay off for the desktop build so it shouldn't break anything.

@pkukol
Copy link
Author

pkukol commented Mar 1, 2017

@dotnet-bot test CentOS7.1 x64 Debug Build and Test
@dotnet-bot test Windows_NT x86 Checked minopts
@dotnet-bot test Ubuntu minopts
@dotnet-bot test OSX minopts

@pkukol
Copy link
Author

pkukol commented Mar 1, 2017

@BruceForstall @pgavlin PTAL if you get a chance. I have to merge the CoreFX test fixes before this goes in, BTW, I'm hoping to get that done soon. Thanks!

@pgavlin
Copy link

pgavlin commented Mar 1, 2017

LGTM at a glance. What has changed since the previous revision?

@pkukol
Copy link
Author

pkukol commented Mar 1, 2017

This is 99% identical to the first go-around, the only material change is a small edit to the GC decoder where it could mess up earlier under rare conditions (plus the updated code is simpler as well so hopefully a win all around - this is in VM/GCinfoDecoder.cpp around line 730); plus, this time I've also pushed a PR in CoreFX to go in first to fix the invalid tests in that repo. Hope that makes sense; don't hesitate to add more comments or suggestions (anyone), though, I am not trying to rush this through and the CoreFX change has to go in first anyway.

@pkukol pkukol merged commit d53b0ff into dotnet:master Mar 1, 2017
@pkukol pkukol deleted the StreamlineMinOptsGCtracking branch March 1, 2017 15:51
jorive pushed a commit to guhuro/coreclr that referenced this pull request May 4, 2017
@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants