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.

Sign up for GitHub

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

Jump to bottom

[MONO] Implement GC.AddMemoryPressure in Mono #82121

Merged
merged 6 commits into from
Feb 18, 2023
Merged

[MONO] Implement GC.AddMemoryPressure in Mono #82121

merged 6 commits into from
Feb 18, 2023

Conversation

naricc
Copy link
Contributor

@naricc naricc commented Feb 14, 2023 •
edited
Loading

This implements GC.AddMemoryPressure in Mono. It basically just copies the heuristics around this API from the CoreCLR implementation and maps it to sgen:

runtime/src/coreclr/vm/comutilnative.cpp

Line 1261 in f857468

void GCInterface::AddMemoryPressure(UINT64 bytesAllocated)
.

Those heuristics may need tuning for mono work loads, but we need some implementation to even begin that work. This seems like a good starting point.

This contributes to missing the missing GC API issue: #73167

@ghost ghost assigned naricc Feb 14, 2023
@ghost
Copy link

ghost commented Feb 14, 2023

Tagging subscribers to this area: @BrzVlad
See info in area-owners.md if you want to be subscribed.

Issue Details

DRAFT

This implements GC.AddMemoryPressure in Mono. It basically just copies the heuristics around this API from the CoreCLR implementation and maps it to sgen:

runtime/src/coreclr/vm/comutilnative.cpp

Line 1261 in f857468

void GCInterface::AddMemoryPressure(UINT64 bytesAllocated)
.

Author: naricc
Assignees: -
Labels:

area-GC-mono
Milestone: -

@naricc naricc force-pushed the naricc/add-memory-pressure branch 2 times, most recently from 032d071 to 1440afe Compare February 14, 2023 19:40
@naricc naricc marked this pull request as ready for review February 14, 2023 20:05
@naricc
Copy link
Contributor Author

naricc commented Feb 14, 2023

@Redth @jonpryor Here is the implementation of AddMemoryPressure in Mono that we discussed briefly in the Cross Team Sync. The CoreCLR version had a bunch of heuristics around it, and I basically just copied those; I suspect we will have to update them as we get a better idea of use cases.

@naricc naricc changed the title [DRAFT][MONO] Implement GC.AddMemoryPressure in Mono [MONO] Implement GC.AddMemoryPressure in Mono Feb 14, 2023
@BrzVlad
Copy link
Member

BrzVlad commented Feb 15, 2023

Isn't adding memory pressure the same as if we had an object allocated with that size. And have the same GC heuristic as if there is more memory allocated ? Although in that case, in the long term it would lead for fewer GCs on mono.

@naricc
Copy link
Contributor Author

naricc commented Feb 15, 2023 •
edited
Loading

Isn't adding memory pressure the same as if we had an object allocated with that size. And have the same GC heuristic as if there is more memory allocated ? Although in that case, in the long term it would lead for fewer GCs on mono.

@BrzVlad I expected it to work that way too. But after looking at the CoreCLR implementation, and talking to @AaronRobinsonMSFT, the existing implementation turns out not to do it that way. Its basically a heuristic around GC.Collect, with some information from the user. If the user keeps AddingMemoryPressure, it may force a collection, if it would not add too much GC overhead. But it doesn't actually change the GCs normal calculation.

Of course, I could make the mono implementation different. But I elected to follow the existing implementation to start with.

@build-analysis build-analysis bot mentioned this pull request Feb 15, 2023
Closed
@naricc naricc force-pushed the naricc/add-memory-pressure branch from ea31496 to c677eae Compare February 15, 2023 22:07
lambdageek
lambdageek reviewed Feb 16, 2023
View reviewed changes
@naricc naricc force-pushed the naricc/add-memory-pressure branch from 12addc6 to f6235fd Compare February 16, 2023 14:54
@lambdageek

This comment was marked as outdated.

Sign in to view
@jonpryor jonpryor mentioned this pull request Feb 16, 2023
Closed
@build-analysis build-analysis bot mentioned this pull request Feb 16, 2023
Closed
@naricc naricc merged commit 0d04df6 into dotnet:main Feb 18, 2023
jonpryor added a commit to jonpryor/java.interop that referenced this pull request Mar 14, 2023
@jonpryor
[generator] Emit partial method for peer constructor
148a553 
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]!

We take our existing "peer constructor" generation code, a'la:

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

and extend it to:

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

This allows a `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: Are there any other places where such `partial` methods would
be useful?  This appears to be the minimum required for this scenario.

[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 mentioned this pull request Mar 14, 2023
Merged
jonpryor added a commit to jonpryor/java.interop that referenced this pull request Mar 14, 2023
@jonpryor
[generator] Emit partial method for peer constructor
054dd3a 
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]!

We take our existing "peer constructor" generation code, a'la:

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

and extend it to:

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

This allows a `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: Are there any other places where such `partial` methods would
be useful?  This appears to be the minimum required for this scenario.

[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
@ghost ghost locked as resolved and limited conversation to collaborators Mar 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@BrzVlad BrzVlad BrzVlad left review comments

@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT left review comments

@lambdageek lambdageek lambdageek approved these changes

@vargaz vargaz Awaiting requested review from vargaz

@thaystg thaystg Awaiting requested review from thaystg thaystg is a code owner

@marek-safar marek-safar Awaiting requested review from marek-safar

Assignees

@naricc naricc

Labels
area-GC-mono
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@naricc @BrzVlad @lambdageek @AaronRobinsonMSFT