Skip to content

[generator] Add support for peerConstructorPartialMethod #1087

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

Merged
merged 1 commit into from
Mar 30, 2023

Conversation

jonpryor
Copy link
Contributor

@jonpryor jonpryor commented Mar 14, 2023

Context: #1085
Context: dotnet/runtime#82121

Some Java objects are big, e.g. Bitmap instances, but as
far as MonoVM is concerned, the Bitmap instances are tiny: a
few pointers, and that's it. MonoVM doesn't know that it could be
referencing several MB of data in the Java VM.

MonoVM is gaining support for GC.AddMemoryPressure() and
GC.RemoveMemoryPressure(), which potentially allows for the
parent of all cross-VM GC integrations: using the GC methods to
inform MonoVM of how much non-managed memory has been allocated.
This could allow MonoVM to collect more frequently when total process
memory is low.

How should we call GC.AddMemoryPressure() and
GC.RemoveMemoryPressure()?

GC.RemoveMemoryPressure() is straightforward: a class can override
Java.Lang.Object.Dispose(bool).

GC.AddMemoryPressure() is the problem: where and how can it be
called from a class binding? This is trickier, as there was no way
to have custom code called by the bound type. Instead, various
forms of "hacky workarounds" are often employed, e.g. copying
generator-emitted content into a partial class, then using
metadata to prevent generator from binding those members.
It's all around fugly.

Fortunately C# has a solution: partial methods!

Add support for a peerConstructorPartialMethod metadata entry,
applicable to <class/> elements, which contains the name of a
partial method to both declare and invoke from the "peer constructor":

<attr
    path="//class[@name='Bitmap']"
    name="peerConstructorPartialMethod"
>_OnBitmapCreated</attr>

This will alter our existing "peer constructor" generation code, a'la:

partial class Bitmap : Java.Lang.Object {
    internal Bitmap (IntPtr h, JniHandleOwnership t) : base (h, t)
    {
    }
}

to instead become:

partial class Bitmap : Java.Lang.Object {
    partial void _OnBitmapCreated ();
    internal Bitmap (IntPtr h, JniHandleOwnership t) : base (h, t)
    {
        _OnBitmapCreated ();
    }
}

This allows a hand-written partial class Bitmap to do:

    // Hand-written code
    partial class Bitmap {
        int _memoryPressure;

        partial void _OnBitmapCreated ()
        {
            _memoryPressure = ByteCount;
            GC.AddMemoryPressure (_memoryPressure);
        }

        protected override void Dispose (bool disposing)
        {
            if (_memoryPressure != 0) {
                GC.RemoveMemoryPressure (_memoryPressure);
                _memoryPressure = 0;
            }
        }
    }

TODO: "extend" this for <method/>s as well?

@jonpryor jonpryor requested a review from jpobst March 14, 2023 09:14
@jonpryor
Copy link
Contributor Author

The compilation overhead of partial methods appears to be negligeable: https://gist.github.com/jonpryor/16b918134fd6fc13b1b20afd10c936cb

jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Mar 14, 2023
@jonpryor jonpryor force-pushed the jonp-ctor-calls-partial-method branch from 148a553 to 054dd3a Compare March 14, 2023 11:21
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Mar 14, 2023
@jonpryor
Copy link
Contributor Author

dotnet/android#7886 looks "green enough" for this to be merge-worthy. This change doesn't appear to break anything.

@@ -52,11 +52,16 @@ public void SetVisibility (string visibility)

public virtual void Write (CodeWriter writer)
{
WritePartialMethodDeclaration (writer);
Copy link
Contributor

Choose a reason for hiding this comment

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

This way of implementation breaks the "spirit" of the SourceWriter model. That is, it is using a MethodWriter to write 2 methods.

This should instead be implemented as a new ConstructorPartialMethod : MethodWriter class in generator.csproj that gets added to the ClassWriter model when the JavaLangObjectConstructor is created.

ie, here:
https://github.com/jonpryor/java.interop/blob/jonp-ctor-calls-partial-method/tools/generator/SourceWriters/BoundClass.cs#L128-L130

Should be something like:

// Add required constructor for all JLO inheriting classes
if (klass.FullName != "Java.Lang.Object" && klass.InheritsObject) {
	Methods.Add (new ConstructorPartialMethod (klass, opt));
	Constructors.Add (new JavaLangObjectConstructor (klass, opt));
}

The only change we should need for MethodWriter is if it doesn't support partial methods in general.

Context: dotnet#1085
Context: dotnet/runtime#82121

Some Java objects are *big*, e.g. [`Bitmap`][0] instances, but as
far as MonoVM is concerned, the `Bitmap` instances are *tiny*: a
few pointers, and that's it.  MonoVM doesn't know that it could be
referencing several MB of data in the Java VM.

MonoVM is gaining support for [`GC.AddMemoryPressure()`][1] and
[`GC.RemoveMemoryPressure()`][2], which potentially allows for the
parent of all cross-VM GC integrations: using the `GC` methods to
inform MonoVM of how much non-managed memory has been allocated.
This could allow MonoVM to collect more frequently when total process
memory is low.

How should we call `GC.AddMemoryPressure()` and
`GC.RemoveMemoryPressure()`?

`GC.RemoveMemoryPressure()` is straightforward: a class can override
`Java.Lang.Object.Dispose(bool)`.

`GC.AddMemoryPressure()` is the problem: where and how can it be
called from a class binding?  This is trickier, as there was no way
to have custom code called by the bound type.  Instead, various
forms of "hacky workarounds" are often employed, e.g. copying
`generator`-emitted content into a `partial` class, then using
`metadata` to *prevent* `generator` from binding those members.
It's all around fugly.

Fortunately C# has a solution: [`partial` methods][3]!

Add support for a `peerConstructorPartialMethod` metadata entry,
applicable to `<class/>` elements, which contains the name of a
`partial` method to both declare and invoke from the "peer constructor":

	<attr
	    path="//class[@name='Bitmap']"
	    name="peerConstructorPartialMethod"
	>_OnBitmapCreated</attr>

This will alter our existing "peer constructor" generation code, a'la:

	partial class Bitmap : Java.Lang.Object {
	    internal Bitmap (IntPtr h, JniHandleOwnership t) : base (h, t)
	    {
	    }
	}

to instead become:

	partial class Bitmap : Java.Lang.Object {
	    partial void _OnBitmapCreated ();
	    internal Bitmap (IntPtr h, JniHandleOwnership t) : base (h, t)
	    {
	        _OnBitmapCreated ();
	    }
	}

This allows a hand-written `partial class Bitmap` to do:

        // Hand-written code
        partial class Bitmap {
            int _memoryPressure;

            partial void _OnBitmapCreated ()
            {
                _memoryPressure = ByteCount;
                GC.AddMemoryPressure (_memoryPressure);
            }

            protected override void Dispose (bool disposing)
            {
                if (_memoryPressure != 0) {
                    GC.RemoveMemoryPressure (_memoryPressure);
                    _memoryPressure = 0;
                }
            }
        }

TODO: "extend" this for `<method/>`s as well?

[0]: https://developer.android.com/reference/android/graphics/Bitmap
[1]: https://learn.microsoft.com/dotnet/api/system.gc.addmemorypressure?view=net-7.0
[2]: https://learn.microsoft.com/dotnet/api/system.gc.removememorypressure?view=net-7.0
[3]: https://learn.microsoft.com/dotnet/csharp/language-reference/keywords/partial-method
@jonpryor jonpryor force-pushed the jonp-ctor-calls-partial-method branch from 054dd3a to 6138fa5 Compare March 30, 2023 17:43
@jonpryor jonpryor changed the title [generator] Emit partial method for peer constructor [generator] Add support for peerConstructorPartialMethod Mar 30, 2023
Copy link
Contributor

@jpobst jpobst left a comment

Choose a reason for hiding this comment

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

Excellent, thanks for the changes!

@jonpryor jonpryor merged commit 909239d into dotnet:main Mar 30, 2023
jonpryor pushed a commit to dotnet/android that referenced this pull request Mar 31, 2023
Fixes: dotnet/java-interop#1090

Changes: dotnet/java-interop@53bfb4a...0355acf

  * dotnet/java-interop@0355acf8: [Xamarin.Android.Tools.Bytecode] Ignore synthetic ctor params (dotnet/java-interop#1091)
  * dotnet/java-interop@909239d4: [generator] Add support for peerConstructorPartialMethod (dotnet/java-interop#1087)
  * dotnet/java-interop@8f3fe625: [generator] Fix an NRE when cloning a method with generic arguments (dotnet/java-interop#1089)

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
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.

2 participants