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

Interface inheritance follow up #85117

Merged
merged 36 commits into from
May 16, 2023

Conversation

jtschuster
Copy link
Member

@jtschuster jtschuster commented Apr 20, 2023

This PR implements the performance improvements outlined here.

It creates types to model the Com interfaces and the methods in the com interfaces. The new types with suffix "Info" are meant to be the first step of the pipeline that can be created with just an ISymbol and SyntaxNode. The types suffixed with "Context" are meant to contain all info necessary for generating the code, and they may require multiple "Info"s to be created (e.g. need info about the base interface or other methods in the interface). There is also a type that represents the Com interface and all the methods that are required to generate the code necessary for Com. Because all this info is in a single type, this could lead to performance issues as a single difference in a method signature would require all code for the type to be regenerated. This should be addressed in the future, but is left as is for the simplicity and flexibility that this model provides.

The generation of the shadowing methods only supports intrinsic C# types (int, string, etc) for the parameters and return values, since it just copies the syntax from the declaration (so any types that are not fully qualified would not be recognized). It also doesn't copy any attributes or refkind keywords from the original declaration.

Generated code diff: https://github.com/jtschuster/GeneratedCode/pull/1/files

@ghost
Copy link

ghost commented Apr 20, 2023

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Issue Details

Creates more types to represent methods, interfaces, etc, and removes symbol references earlier in the pipeline. This should make it easier to generate shadowing methods for inherited methods.

Will be making a more in depth description soon.

Author: jtschuster
Assignees: -
Labels:

area-System.Runtime.InteropServices

Milestone: -

Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

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

Is there a way we can ensure that changing the signature of one method on the interface doesn't cause us to regenerate all of the methods on the interface? It looks like as of now, this PR changes the pipeline such that any change to a valid method causes us to regenerate all of the stubs for the interface (not just for the method that changed).

@jtschuster
Copy link
Member Author

Is there a way we can ensure that changing the signature of one method on the interface doesn't cause us to regenerate all of the methods on the interface? It looks like as of now, this PR changes the pipeline such that any change to a valid method causes us to regenerate all of the stubs for the interface (not just for the method that changed).

Which part of the pipeline do you mean by "regenerate the stubs for the interface"? I'm not sure if we had that behavior previously for calls to GenerateImplementationVtable and GenerateImplementationVTableMethods. The pipeline can definitely be tweaked get that behavior and to make incremental changes much cheaper, like flattening the ComInterfaceContext where possible and breaking up expensive calculations into separate steps. I'm just a little worried about trying to optimize too much before we generate shadowing methods and get an idea of what is the most expensive parts and have some kind of way to test whether we run incrementally.

@jkoritzinsky
Copy link
Member

I was thinking of the GenerateManagedToUnmanagedStub and GenerateUnmanagedToManagedStub steps in particular, following from the CalculateStubInformation step.

I'll focus on adding some incrementality tests this week so we can see where we're at and validate whatever changes we want to make.

@@ -255,7 +272,7 @@ private static MemberDeclarationSyntax GenerateIUnknownDerivedAttributeApplicati
.AddAttributeLists(AttributeList(SingletonSeparatedList(s_iUnknownDerivedAttributeTemplate))));

// Todo: extract info needed from the IMethodSymbol into MethodInfo and only pass a MethodInfo here
private static IncrementalMethodStubGenerationContext CalculateStubInformation(MethodDeclarationSyntax syntax, IMethodSymbol symbol, int index, StubEnvironment environment, CancellationToken ct)
private static IncrementalMethodStubGenerationContext CalculateStubInformation(MethodDeclarationSyntax syntax, IMethodSymbol symbol, int index, StubEnvironment environment, ManagedTypeInfo typeKeyOwner, CancellationToken ct)
Copy link
Member Author

Choose a reason for hiding this comment

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

TypeKeyOwner will be the deriving type for shadowing methods and cannot just be the containingType of the methods

// TODO: Copy full name of parameter types and attributes / attribute arguments for parameters
return MethodInfo.Syntax
.WithModifiers(TokenList(Token(SyntaxKind.NewKeyword)))
.WithExpressionBody(
Copy link
Member Author

Choose a reason for hiding this comment

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

Arrow expression body makes it easier to handle void methods - no need to worry about the return keyword being there or not.

Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

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

The generated code looks good, but I'm concerned about the incrementality of the pipeline after this PR, as well as the fact that it looks like there's some now-unused code in this PR.

Instead of centralizing the CalculateStubInformation calls in ComInterfaceAndMethodsContext, could we use the GroupContextsForInterfaceGeneration concept to implement shadowing method generation?

Could we have a model where (based on the version of ComInterfaceGenerator in main) we combine the interfacesWithMethods info with the interfaceBaseInfo to get the collection of shadowing methods at that stage and process them down the existing pipeline (with a mechanism to pass down the base type to use instead of always using the ContainingType)?

That way we'd preserve the incrementality that we already have today and avoid causing a "regenerate the world" scenario when one method is changed.

@jtschuster
Copy link
Member Author

I reworked some things to avoid doing so much in the CalculateAllMethods methods. It uses a new Builder type to hold all the info except the StubContext before that is calculated in a separate step. This lets us have a couple steps before the CalculateMethodStub that can be incremental.

I also modified the GroupMethodsForGeneration to also take a list of interfaces, which lets us group things the same way and return a ComInterfaceAndMethodsContext with an empty method array for "marker interfaces". This way we don't need to handle them separately.

We also don't need separate inherited and shadowing methods, so I combined them into "ShadowingMethods".

Still might be some cleanup to do, but this should have the outline of the plan.

@jtschuster
Copy link
Member Author

Ready for re-review

Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

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

This is looking much better! Just a few comments. Once those (and the TODOs) are addressed, this will be ready for merge!

jtschuster and others added 3 commits May 15, 2023 14:00
Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

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

One last issue I noticed in the generated code on a final lookover.

@jtschuster jtschuster merged commit ff92d4e into dotnet:main May 16, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants