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

JIT: test case showing inexact type for this in constructors #16239

Merged
merged 1 commit into from
Feb 7, 2018

Conversation

AndyAyersMS
Copy link
Member

@AndyAyersMS AndyAyersMS commented Feb 6, 2018

Add test case showing that in constructors the exact type of this is not generally known. To devirtualize we need to rely on type propagation from the newobj.

Related discussion in #16198.

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Feb 6, 2018

@briansull PTAL
cc @dotnet/jit-contrib

[Edit: results below are for a now-abandoned invalid opt, so please disregard]

jit-diffs results:

Total bytes of diff: -101 (0.00% of base)
    diff is an improvement.
Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes
Top file regressions by size (bytes):
         277 : System.Text.Encoding.CodePages.dasm (0.39% of base)
          10 : System.Private.CoreLib.dasm (0.00% of base)
Top file improvements by size (bytes):
        -102 : Microsoft.CodeAnalysis.VisualBasic.dasm (0.00% of base)
         -76 : Microsoft.CodeAnalysis.CSharp.dasm (0.00% of base)
         -32 : Newtonsoft.Json.dasm (-0.01% of base)
         -27 : System.Private.Xml.dasm (0.00% of base)
         -21 : System.Collections.NonGeneric.dasm (-0.08% of base)
18 total files with size differences (16 improved, 2 regressed), 112 unchanged.
Top method regessions by size (bytes):
          78 : System.Text.Encoding.CodePages.dasm - ISCIIDecoder:.ctor(ref):this
          57 : System.Text.Encoding.CodePages.dasm - ISO2022Encoder:.ctor(ref):this
          40 : System.Text.Encoding.CodePages.dasm - ISCIIEncoder:.ctor(ref):this
          30 : System.Private.CoreLib.dasm - System.Text.ASCIIEncoding:.ctor():this
          28 : System.Text.Encoding.CodePages.dasm - System.Text.EncoderNLS:.ctor(ref):this
Top method improvements by size (bytes):
         -41 : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.Symbols.SourceUserDefinedOperatorSymbolBase:.ctor(int,ref,ref,ref,ref,ref,struct,ref,bool):this
         -31 : Microsoft.CodeAnalysis.VisualBasic.dasm - AnonymousDelegateTemplateSymbol:.ctor(ref,struct):this
         -27 : Microsoft.CodeAnalysis.VisualBasic.dasm - Microsoft.CodeAnalysis.VisualBasic.Symbols.SourceEventSymbol:.ctor(ref,ref,ref,ref,ref):this
         -26 : Microsoft.CodeAnalysis.VisualBasic.dasm - Microsoft.CodeAnalysis.VisualBasic.Symbols.SourceNamedTypeSymbol:.ctor(ref,ref,ref):this
         -18 : System.Private.Xml.dasm - System.Xml.XmlEncodedRawTextWriter:.ctor(ref,ref):this (2 methods)
60 total methods with size differences (44 improved, 16 regressed), 154780 unchanged.

Copy link
Member

@erozenfeld erozenfeld left a comment

Choose a reason for hiding this comment

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

LGTM

@jkotas
Copy link
Member

jkotas commented Feb 6, 2018

Is the following still going to print "Child" with this optimization?

using System;
using System.Threading;
using System.Threading.Tasks;

class Test {

public Test() {
    Console.WriteLine(ToString());
}

static void Main() {
     new Child();
}

}

class Child : Test {

public override string ToString() {
    return "Child";
}

}

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Feb 6, 2018

Yes, it does.

That particular call was already being devirtualized because of inlining into Main and type propagation from newobj.

In cases where we don't inline the constructor, this change kicks in.

[edited to fix a few typos...]

@AndyAyersMS
Copy link
Member Author

@jkotas thought about it some more and you were right.

We also need to check that the constructor type is final. Relevant example.

using System;
using System.Runtime.CompilerServices;

class Test {

    public override string ToString() {
        return "Test";
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    public Test() {
        Console.WriteLine(ToString());     // we don't know type of "this" exactly here
    }
    
    static void Main() {
        new Child();
    }
}

class Child : Test {

    [MethodImpl(MethodImplOptions.NoInlining)]
    public Child() { }

    public override string ToString() {
        return "Child";
    }
}

Looks like I should add a test in addition to a fix.

@AndyAyersMS
Copy link
Member Author

And since we already know about final classes the upshot is this now doesn't tell us anything we don't already know.

So I'll update this to just add a test case as we don't seem to have any basic tests that would catch this.

@AndyAyersMS AndyAyersMS changed the title JIT: recognize we have exact type for this in constructors JIT: test case showing inexact type for this in constructors Feb 6, 2018
@AndyAyersMS
Copy link
Member Author

Tizen failure unrelated, will ignore.

@AndyAyersMS AndyAyersMS merged commit ff1da90 into dotnet:master Feb 7, 2018
@AndyAyersMS AndyAyersMS deleted the ExactTypeInCtor branch February 7, 2018 00:05
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.

3 participants