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

Use C#7 is expression to remove additional cast #32131

Open
wants to merge 1 commit into
base: master
from

Conversation

@Kahbazi
Copy link

Kahbazi commented Sep 6, 2018

No description provided.

@ViktorHofer

This comment has been minimized.

Copy link
Member

ViktorHofer commented Sep 6, 2018

Based on @jnm2's comment let us close this for now and revisit it when C#8 is being used in corefx.

@ViktorHofer ViktorHofer closed this Sep 6, 2018

@karelz karelz added this to the 3.0 milestone Sep 6, 2018

@Kahbazi

This comment has been minimized.

Copy link

Kahbazi commented Dec 17, 2018

@ViktorHofer Can we reopen this PR since now C# 8.0 is being used?

dotnet/core@1fdca14

@ViktorHofer

This comment has been minimized.

Copy link
Member

ViktorHofer commented Dec 17, 2018

Sure

@ViktorHofer ViktorHofer reopened this Dec 17, 2018

@karelz

karelz approved these changes Dec 19, 2018

@ViktorHofer

This comment has been minimized.

Copy link
Member

ViktorHofer commented Dec 19, 2018

@dotnet-bot test this please

@ViktorHofer

This comment has been minimized.

Copy link
Member

ViktorHofer commented Dec 19, 2018

No, we can't merge PRs which didn't run recently as that can break builds. We had that once in the past and I definitely don't want to see that again...

@stephentoub

This comment has been minimized.

Copy link
Member

stephentoub commented Dec 20, 2018

Has the originally cited issue actually been addressed (e.g. is the corefx repo actually building with a compiler that has the mentioned fix)? And if yes, does this PR actually make a positive impact on the resulting IL or asm? At the moment at least it seems to increase the size of the IL, with no discernable impact on performance.

@Kahbazi

This comment has been minimized.

Copy link

Kahbazi commented Dec 22, 2018

@stephentoub you are right using is-expression will increase the size of IL but on the other hand it will decrease the Jit Assembly and has fewer operations.

using System;
public class C 
{
    public void IsExpression(A a) 
    {
    	if (a is B b)
    	    Console.WriteLine(b);
    }
    
    public void Cast(A a) 
    {
    	if (a is B)
            Console.WriteLine((B)a);
    }
}

public class A {}

public class B : A {}
C.IsExpression(A)
    L0000: mov ecx, 0x1b0b176c
    L0005: call 0x73cfc540
    L000a: test eax, eax
    L000c: jz L0015
    L000e: mov ecx, eax
    L0010: call System.Console.WriteLine(System.Object)
    L0015: ret

C.Cast(A)
    L0000: push esi
    L0001: mov esi, edx
    L0003: mov edx, esi
    L0005: mov ecx, 0x1b0b176c
    L000a: call 0x73cfc540
    L000f: test eax, eax
    L0011: jz L001a
    L0013: mov ecx, esi
    L0015: call System.Console.WriteLine(System.Object)
    L001a: pop esi
    L001b: ret
@danmosemsft

This comment has been minimized.

Copy link
Member

danmosemsft commented Dec 22, 2018

The generated code looks inefficient. Could you open a bug in the coreclr repo for that? In general we prefer not to work around codegen unless we have data demonstrating its perf critical and it's clear there isn't going to be a near term fix to the codegen.

@Kahbazi

This comment has been minimized.

Copy link

Kahbazi commented Dec 23, 2018

@danmosemsft Just so the issue that I want to open is more clear to me, based on https://sharplab.io/#v2:EYLgZgpghgLgrgJwgZwLQQHYAsoYMYQAmqcyAlhgOYA+AAgAwAEtAjANwCwAULQMzMAmRgGFG3AN7dG05v1oAWRgElkAUQAeAByTJyAewwAKAIKMoASjFcZjSdZkBIMmEaGojMskYAhRsHNSjjasAJyG/pz20gC+gdJxssyKwlDIMCZmlgl2Nk4ubh5e3gFRNmWhhobFFpE2sVz13HyCjKbijTxyQr4grbbRQA== this is what happens to is-expression

C# 8

public void IsExpression(A a) 
{
    if (a is B b)
        Console.WriteLine(b);
}

C#

public void IsExpression(A a)
{
    B value;
    if ((value = (a as B)) != null)
    {
        Console.WriteLine(value);
    }
}

IL

    .method public hidebysig 
        instance void IsExpression (
            class A a
        ) cil managed 
    {
        // Method begins at RVA 0x2050
        // Code size 17 (0x11)
        .maxstack 2
        .locals init (
            [0] class B
        )

        IL_0000: ldarg.1
        IL_0001: isinst B
        IL_0006: dup
        IL_0007: stloc.0
        IL_0008: brfalse.s IL_0010

        IL_000a: ldloc.0
        IL_000b: call void [mscorlib]System.Console::WriteLine(object)

        IL_0010: ret
    } // end of method C::IsExpression

JIT Asm

C.IsExpression(A)
    L0000: mov ecx, 0x1b4b1784
    L0005: call 0x73cfc540
    L000a: test eax, eax
    L000c: jz L0015
    L000e: mov ecx, eax
    L0010: call System.Console.WriteLine(System.Object)
    L0015: ret

The generated code in which one of these steps is not inefficient? I'm guessing JIT since you've mentioned coreclr, but I just want to be sure.

@danmosemsft

This comment has been minimized.

Copy link
Member

danmosemsft commented Dec 23, 2018

Yes codegen is what we often call the component that generates code at JIT time. @jkotas do you agree the codegrn could be improved here? I am far from knowledgeable here but this seems unnecessary for example

L0001: mov esi, edx
    L0003: mov edx, esi
@Kahbazi

This comment has been minimized.

Copy link

Kahbazi commented Dec 23, 2018

Thanks, I see your point. I will did create an issue on coreclr. dotnet/coreclr#21660

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Dec 24, 2018

@jkotas do you agree the codegen could be improved here

Yes, it has been a known issue. The JIT does not handle the IL that Roslyn generates for these patterns well.

I know folks have been doing similar kinds of cosmetic cleanups quite a bit.

  • I have been rejecting them in small performance critical methods where the worse codegen would have material impact. Those can be done only once the related JIT issues are fixed.
  • I have been accepting them in methods where performance does not matter. This one is in Linq. Linq is big and slow, so I would have a problem accepting this one. The worse codegen is unlikely to matter even if it takes a while to fix the underlying JIT issues.

@Kahbazi Kahbazi changed the title Use C#7 is operator to remove additional cast Use C#7 is expression to remove additional cast Dec 25, 2018

@Kahbazi

This comment has been minimized.

Copy link

Kahbazi commented Jan 8, 2019

@ViktorHofer Is there anything that I should do for this PR to get merged?

@ViktorHofer

This comment has been minimized.

Copy link
Member

ViktorHofer commented Jan 8, 2019

I think @danmosemsft and @stephentoub are the right people to answer that.

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