Skip to content

Add source generator for UnbindAllBindables #5764

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented Apr 29, 2023

Prereqs:

Because UnbindAllBindables() is virtual and has been overridden, I've had to make another method InternalUnbindAllBindables() for the SGen.

Before:

Method Mean Error StdDev Gen 0 Allocated
TestBaseline 0.0000 ns 0.0000 ns 0.0000 ns - -
TestCompositeDrawable 2,166.6177 ns 43.0838 ns 44.2439 ns 0.0191 1,640 B
TestContainer 2,571.1976 ns 51.2559 ns 68.4252 ns 0.0191 1,712 B
TestTypeNestedComposite 2,874.6742 ns 57.4976 ns 94.4702 ns 0.0191 1,640 B
TestComplexComposite 3,241.3708 ns 51.8630 ns 53.2595 ns 0.0305 2,672 B

After:

Method Mean Error StdDev Gen 0 Allocated
TestBaseline 0.0000 ns 0.0000 ns 0.0000 ns - -
TestCompositeDrawable 2,069.2690 ns 31.1794 ns 27.6397 ns 0.0191 1,640 B
TestContainer 2,440.0966 ns 12.3803 ns 9.6658 ns 0.0191 1,712 B
TestTypeNestedComposite 2,668.8426 ns 42.0074 ns 39.2937 ns 0.0191 1,640 B
TestComplexComposite 2,589.7359 ns 50.5762 ns 47.3090 ns 0.0305 2,672 B

(note that this doesn't account for reflection memory usage)

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

Code quality inspections need disabling, probably.

{
public interface ISourceGeneratedUnbindAllBindables
{
protected internal Type KnownType { get; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I notice that this seems to be the same thing as in #5761 (comment) again. This means that it is possible for a class to receive two KnownType properties from two disparate sourcegen interfaces with the same value, so I do wonder whether we want to be implementing these case-by-case rather than splitting this one member into a separate interface.

Not sure how much we care about code size but it does look like a potential excess. We may start paying for this in JIT times which we're not going to be able to shave off or hide that easily. On the other side, I also can understand that implementing this to be done as a common thing may not fit in very well into the current syntax target/emitter abstraction.

{
protected internal Type KnownType { get; }

void InternalUnbindAllBindables();
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Internal qualifier being a prefix rather than a suffix (as we usually do elsewhere) is slightly annoying me here. How intentional was this?

{
}

protected override bool CheckValid(INamedTypeSymbol symbol) => isDrawableSubType(symbol);
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this means that the generator is going to emit code for every drawable subtype, even if it doesn't introduce any new bindables to unbind? Do we care, and if we do, how much of a pain is it to have it only emit when necessary?

Comment on lines +75 to +76
private bool isDrawableType(INamedTypeSymbol type)
=> type.Name == "Drawable" && SyntaxHelpers.GetFullyQualifiedTypeName(type) == "osu.Framework.Graphics.Drawable";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Repeat of #5761 (comment). Should probably be moved out to SyntaxHelpers at this stage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: On hold
Development

Successfully merging this pull request may close these issues.

2 participants