Skip to content
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

Default interface member consumption #8628

Merged
merged 34 commits into from Mar 17, 2020

Conversation

@TIHan
Copy link
Contributor

TIHan commented Feb 25, 2020

This adds the consumption of default interface methods. This is necessary in order to interop with C# code. See here: #6779

Design: fsharp/fslang-design#372

Closing old PR, #7025, in favor of this one.

@TIHan TIHan mentioned this pull request Feb 25, 2020
3 of 4 tasks complete
TIHan added 8 commits Feb 25, 2020
@TIHan TIHan changed the title [WIP] Default interface method consumption [WIP] Default interface member consumption Feb 27, 2020
TIHan added 2 commits Feb 27, 2020
@TIHan TIHan changed the title [WIP] Default interface member consumption Default interface member consumption Feb 27, 2020
@TIHan

This comment has been minimized.

Copy link
Contributor Author

TIHan commented Feb 27, 2020

@cartermp, @dsyme, @jonsequitur - This is done and just needs a review and some testing if you all can.

Copy link
Member

KevinRansom left a comment

LGTM

Copy link
Contributor

cartermp left a comment

There's multiple places where something is n^2, two of which already existed. Is there cause for concern here?

) g amap m allowMultiIntfInst ty []
|> List.concat
|> List.groupBy (fun minfo -> (minfo.LogicalName, minfo.NumArgs, minfo.GenericArity))
|> List.map (fun (_, minfos) -> GetTopHierarchyItemsByType g amap (fun (minfo: MethInfo) -> Some (minfo.ApparentEnclosingType, m)) minfos)

This comment has been minimized.

Copy link
@cartermp

cartermp Feb 28, 2020

Contributor

Since it's being mapped onto a list, the o(n^2) function is really making this a O(n^3) operation

This comment has been minimized.

Copy link
@TIHan

TIHan Feb 28, 2020

Author Contributor

I don't think so? GetTopHierarchyItemsByType is taking minfos from each item of the results from folding the hierarchy. There could be two items, but each item could have 10 minfos.

Also note that the results of this are memoized.

This comment has been minimized.

Copy link
@cartermp

cartermp Feb 28, 2020

Contributor

IIRC that's still O(n^3) - for every item in a list, we're doing an O(n^2) operation. The list could not be large in practice, nor could the inner minfos. But I think it's good to just have a think about this, given some literature on the subject:

https://randomascii.wordpress.com/2019/12/08/on2-again-now-in-wmi/
https://accidentallyquadratic.tumblr.com/

Not a pushback here if we have strong confidence that these values won't get too large. But I'm just cautious, especially given bug reports the C# team has seen in the past about generated code with thousands of overloads. If massive interfaces are seen in the future, how much could this torture the compiler?

This comment has been minimized.

Copy link
@KevinRansom

KevinRansom Feb 28, 2020

Member

@cartermp, I think Will is right, it's probably not going to be that huge although it seems like something we should keep an eye on.

This comment has been minimized.

Copy link
@TIHan

TIHan Mar 2, 2020

Author Contributor

If it makes us feel better, I can create a test with a lot of overloads and compare perf withi this PR and without. Should be straight forward.

This comment has been minimized.

Copy link
@dsyme

dsyme Mar 3, 2020

Contributor

The complexity should be reduced here as a matter of principle I think. O(n^3) is always trouble sooner or later

And as I mention down below the overall complexity is at least O(n^4) or O(n^5) since this function is called inside a per-method loop (which might itself be called in a loop over each interface I think, need to check).

src/fsharp/LanguageFeatures.fs Outdated Show resolved Hide resolved
src/fsharp/MethodOverrides.fs Outdated Show resolved Hide resolved
@TIHan

This comment has been minimized.

Copy link
Contributor Author

TIHan commented Feb 28, 2020

@cartermp There are some places that introduce O(N^2), but also removed a case that was O(N^2). The large amount of work is done in InfoReader and those results are cached.

I'm not too worried about performance. I don't know if it is slower or faster though. If you have 10,000 different types, each with different hierarchies of different interface types, then there could be a perf problem. Most of the code paths that you see changed, only occur when you actually implement an interface on a class.

@TIHan

This comment has been minimized.

Copy link
Contributor Author

TIHan commented Feb 28, 2020

Probably the best thing to do ensure compiler perf, is to simply test before and after.

Copy link
Contributor

cartermp left a comment

Looked over the parts of the RFC and saw correct corresponding tests for all them. Great work!

Copy link
Contributor

dsyme left a comment

One more general comment: The structure of the whole abstract slot checking code is to

  1. gather then slots that need to be implemented

  2. gather the implementations provided

  3. check they add up.

You're modifying (1) by also gathering the default interface implementations, which is fair enough as it would be just invasive to modify (2). However you do this on a method-by-method basis, one by one, searching the hierarchy for the relevant method impl for each method. It feels that, structurally, gathering the default interface implementations should be done in bulk - just a single fold over the heirarchy that gets all of them from all the most specific interface types all in one pass, then make a table of them (MultiMap?) keyed by method name, arity, generic arity to extract out the flag information.

| Some (yTy, _) ->
if typeEquiv g xTy yTy then true
else not (TypeFeasiblySubsumesType 0 g amap m xTy CanCoerce yTy)) then
yield x ]

This comment has been minimized.

Copy link
@dsyme

dsyme Mar 3, 2020

Contributor

The nested if and indentation is hard to read. I think the condition should be separated

            let alreadySeen =
                xs |> List.exists (fun y ->
                    match f y with
                    | None -> false
                    | Some (yTy, _) ->
                        (not (typeEquiv g xTy yTy) && TypeFeasiblySubsumesType 0 g amap m xTy CanCoerce yTy))
            if not alreadySeen then
                yield x ]
@@ -331,6 +351,52 @@ type InfoReader(g: TcGlobals, amap: Import.ImportMap) =
ty
None

let GetIntrinsicTopInterfaceOverrideMethodSetsUncached ((optFilter, ad, allowMultiIntfInst), m, ty) =

This comment has been minimized.

Copy link
@dsyme

dsyme Mar 3, 2020

Contributor

Add a /// comment on this please

/// Visiting each type in the hierarchy and accumulate methods that are the OverrideBy target of some MethodImpl on that type 

I believe the whole thing can be rewritten, e.g. https://gist.github.com/dsyme/f26e78adeadbf7d3802ba162300d33fe

Note you can assert that optFilter is set.

The filter and grouping on (minfo.LogicalName, minfo.NumArgs, minfo.GenericArity) is not right and can be removed. I think it can just be removed as this is always followed by an exact signature match https://github.com/dotnet/fsharp/pull/8628/files#diff-87f5261ee04fbbcd39ff79a83af01e01R433. The (LogicalName, NumArgs, GenericArity tuple doesn't identify method signatures uniquely - just approximately - because types aren't being compared. Just remove it since the IsExactSigMatch follows

Also the "MostSpecificType" search at the end is not needed here as the only consuming site also does the search. It is better removed from here is it is being done before the exact signature match which is surely not correct.

@@ -331,6 +351,52 @@ type InfoReader(g: TcGlobals, amap: Import.ImportMap) =
ty
None

let GetIntrinsicTopInterfaceOverrideMethodSetsUncached ((optFilter, ad, allowMultiIntfInst), m, ty) =

let checkMethod (minfo: MethInfo) (ilMethRef: ILMethodRef) =

This comment has been minimized.

Copy link
@dsyme

dsyme Mar 3, 2020

Contributor

I recommend lifting this out so it's not a nested function, and add a /// comment about what it's doing.

Also I think it's clearer if this method takes an ILMethodImpl as second parameter (instead of an ILMethodRef) as then it's clearer what relationship is being checked.

The name of the function could be checkMethodImplIsPartialMatchForInterfaceMethod? I think it's important to include the word Partial as the checks are not complete - later an exact signature match is needed to check argument type matches

) g amap m allowMultiIntfInst ty []
|> List.concat
|> List.groupBy (fun minfo -> (minfo.LogicalName, minfo.NumArgs, minfo.GenericArity))
|> List.map (fun (_, minfos) -> GetTopHierarchyItemsByType g amap (fun (minfo: MethInfo) -> Some (minfo.ApparentEnclosingType, m)) minfos)

This comment has been minimized.

Copy link
@dsyme

dsyme Mar 3, 2020

Contributor

The complexity should be reduced here as a matter of principle I think. O(n^3) is always trouble sooner or later

And as I mention down below the overall complexity is at least O(n^4) or O(n^5) since this function is called inside a per-method loop (which might itself be called in a loop over each interface I think, need to check).

// If the OverrideBy method is found, add it to the method set for that type in the hierarchy.
(acc, tcref.ILTyconRawMetadata.MethodImpls.AsList)
||> List.fold (fun acc ilMethImpl ->
let optFilter2 =

This comment has been minimized.

Copy link
@dsyme

dsyme Mar 3, 2020

Contributor

This optFilter code looks smelly - see the rewrite of all this, as you can asssume the input optFilter is set.

acc)

if List.isEmpty minfos then acc
else minfos :: acc

This comment has been minimized.

Copy link
@dsyme

dsyme Mar 3, 2020

Contributor

Since you List.concat immediately after this whole operation you can just do an append here minfos @ acc - and then you don't need either the if List.isEmpty or the List.concat. See the rewrite

minfo.IsFinal &&
minfo.IsVirtual &&
minfo.IsInterfaceMethod &&
ilMethRef.ArgCount = (match minfo.NumArgs with [] -> 0 | count :: _ -> count) &&

This comment has been minimized.

Copy link
@dsyme

dsyme Mar 3, 2020

Contributor

Use

        ilMethRef.ArgCount = List.sum minfo.NumArgs 

let checkMethod (minfo: MethInfo) (ilMethRef: ILMethodRef) =
minfo.IsFinal &&
minfo.IsVirtual &&

This comment has been minimized.

Copy link
@dsyme

dsyme Mar 3, 2020

Contributor

IsInterfaceMethod will check IsVirtual I believe

if (optFilter.IsSome && optFilter2.IsSome) || optFilter.IsNone then
let minfos =
([], GetImmediateIntrinsicMethInfosOfType (optFilter2, ad) g amap m ty)
||> List.fold (fun acc minfo ->

This comment has been minimized.

Copy link
@dsyme

dsyme Mar 3, 2020

Contributor

The minfo on this line shadows the outer minfo. Could you change the names to clarify? Or see the proposed rewrite

@@ -205,7 +225,7 @@ type HierarchyItem =

/// An InfoReader is an object to help us read and cache infos.
/// We create one of these for each file we typecheck.
type InfoReader(g: TcGlobals, amap: Import.ImportMap) =
type InfoReader(g: TcGlobals, amap: Import.ImportMap) as this =

This comment has been minimized.

Copy link
@dsyme

dsyme Mar 3, 2020

Contributor

The as this should not be needed - make the method that consumes it into a member that has access to this

This comment has been minimized.

Copy link
@TIHan

TIHan Mar 13, 2020

Author Contributor

Unfortunately, it's not that easy. I need access to 'this' because I want to perform a lazy operation of collecting runtime feature support info which requires looking at fields.

TIHan added 8 commits Mar 3, 2020
…rp into feature/dim-consumption
@TIHan

This comment has been minimized.

Copy link
Contributor Author

TIHan commented Mar 10, 2020

Moving through the feedback.

I need to update the RFC design to include static interface methods.

TIHan added 4 commits Mar 12, 2020
@TIHan

This comment has been minimized.

Copy link
Contributor Author

TIHan commented Mar 13, 2020

@cartermp @dsyme This is ready for review again.

Highlights of change:

  • Picking up all most specific override interface methods once before checking a type construct. Returns a NameMultiMap and gets used when checking for interface method implementations.
  • Part of the work done to get the most specific override interface methods is cached.
TIHan added 2 commits Mar 13, 2020
@TIHan

This comment has been minimized.

Copy link
Contributor Author

TIHan commented Mar 13, 2020

I added another test that is currently failing. Basically, how I group the methods by name and check their signature for most specific, I did not account for methods that don't technically override but instead shadow with the exact same signature. Will need to do more work here.

TIHan added 3 commits Mar 14, 2020
@dsyme
dsyme approved these changes Mar 17, 2020
Copy link
Contributor

dsyme left a comment

This looks really great, much cleaner!

@KevinRansom KevinRansom merged commit 91bd7da into dotnet:master Mar 17, 2020
15 checks passed
15 checks passed
WIP Ready for review
Details
fsharp-ci Build #20200316.20 succeeded
Details
fsharp-ci (Build EndToEndBuildTests) Build EndToEndBuildTests succeeded
Details
fsharp-ci (Build Linux) Build Linux succeeded
Details
fsharp-ci (Build Linux_FCS) Build Linux_FCS succeeded
Details
fsharp-ci (Build MacOS) Build MacOS succeeded
Details
fsharp-ci (Build MacOS_FCS) Build MacOS_FCS succeeded
Details
fsharp-ci (Build SourceBuild_Windows) Build SourceBuild_Windows succeeded
Details
fsharp-ci (Build UpToDate_Windows) Build UpToDate_Windows succeeded
Details
fsharp-ci (Build Windows coreclr_release) Build Windows coreclr_release succeeded
Details
fsharp-ci (Build Windows desktop_release) Build Windows desktop_release succeeded
Details
fsharp-ci (Build Windows fsharpqa_release) Build Windows fsharpqa_release succeeded
Details
fsharp-ci (Build Windows vs_release) Build Windows vs_release succeeded
Details
fsharp-ci (Build Windows_FCS) Build Windows_FCS succeeded
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.