Mark types needed for instance checks only if they are ever instantiated

[marek-safar]

No description provided.

[mrvoorhe]

This seems rather niche. Is there some specific scenario it helps with? Does it help reduce size in general? Just asking out of curiosity so that I understand it's impact.

[marek-safar]

This seems rather niche. Is there some specific scenario it helps with? Does it help reduce size in general?

It works in general, few large size samples I tried saved about 3%

[MichalStrehovsky]

It works in general, few large size samples I tried saved about 3%

Oh, that's a pretty cool win. If one has is/as in code, the code will usually go like

if (foo is Bar)
{
    ((Bar)foo).SomeMethodOnBar();
}

if (foo is Bar b)
{
    b.SomeMethodOnBar();
}

And SomeMethodOnBar will bring in Bar anyway. I would have expected this to save something only if we were to couple this with constant propagation and eliminate the unreachable branches. It's nice that we can get wins with a cheaper feature - I wouldn't even try to explore this without the constant propagation part :)

[marek-safar]

I would have expected this to save something only if we were to couple this with constant propagation and eliminate the unreachable branches.

That's the next level (on my list too for later) but this is useful even without it as the type itself can bring large dependencies. Few examples where this kicks in

• https://github.com/dotnet/runtime/blob/71283319d0bf0f8c9132e117419fc2294342b510/src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackTrace.cs#L407

• https://github.com/dotnet/runtime/blob/6072e4d3a7a2a1493f514cdf4be75a3d56580e84/src/libraries/System.Private.CoreLib/src/System/Activator.RuntimeType.cs#L24

[mrvoorhe]

It's not clear to me if this could alter static constructor execution, but it sounds like there is concern that it could?

If the potential exists for this to impact static constructor execution my preference would be to put this behavior behind a new CodeOptimizations value. I'm fine with it being enabled by default. It's just nice to have a way to switch subtle behavior changing things off. As @MichalStrehovsky pointed out, someone could burn hours and hours looking at a bug related to the linker changing the behavior of .cctor's.

[mrvoorhe]

It works in general, few large size samples I tried saved about 3%

I think you are catching some niche scenario that let's you knock out a large section of unused code. Our code size tests are showing only slightly above 0% reduction.

---------------------------
Differences
---------------------------
UnityEngine.dll (WIN)
  Expected: 26112
  Actual: 25600
  Diff: -512 (-1.9608%)
UnityEngine.IMGUIModule.dll (WIN)
  Expected: 44032
  Actual: 43520
  Diff: -512 (-1.1628%)
---------------------------
Total (WIN)
  Expected: 2806784
  Actual: 2805760
  Diff: -1024 (-0.0365%)

That's not to say this isn't worth doing. It certainly is if it helps some case by 3%. 3% is hard to come by. But based on this I would say that I would definitely like to see a CodeOptimization flag if this can alter .cctor behavior. I don't want to waste hours debugging a .cctor behavior change over .03% code size improvement 😄

[marek-safar]

It's not clear to me if this could alter static constructor execution, but it sounds like there is concern that it could?

That concern was addressed.

[vitek-karas]

Sorry for being late to the party... just a couple of minor comments.

[vitek-karas]

Nit: member fields start with _ in this class - so to keep to consistent it should be _pending_isinst_instr.

[vitek-karas]

I would also add a test where the class T6 is used in is but also where a static member on T6 is called. So the class is not instantiated, but it will be kept. I think we will still rewrite the is in that case.

[vitek-karas]

I would also add one test which uses as - I know it translates to the same instruction, but it would be good to have it for completeness.