Skip to content

[MONO] Implement GC.AddMemoryPressure in Mono - #82121

Merged
naricc merged 6 commits into
dotnet:mainfrom
naricc:naricc/add-memory-pressure
Feb 18, 2023
Merged

[MONO] Implement GC.AddMemoryPressure in Mono#82121
naricc merged 6 commits into
dotnet:mainfrom
naricc:naricc/add-memory-pressure

Conversation

@naricc

@nariccnaricc commented Feb 14, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

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

voidGCInterface::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

@ghostghost added the area-GC-mono label Feb 14, 2023
@ghostghost assigned nariccFeb 14, 2023
@ghost

ghost commented Feb 14, 2023

Copy link
Copy Markdown

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

voidGCInterface::AddMemoryPressure(UINT64 bytesAllocated)
.

Author:naricc
Assignees:-
Labels:

area-GC-mono

Milestone:-

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

naricc commented Feb 14, 2023

Copy link
Copy Markdown
ContributorAuthor

@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.

@nariccnaricc changed the title [DRAFT][MONO] Implement GC.AddMemoryPressure in Mono[MONO] Implement GC.AddMemoryPressure in MonoFeb 14, 2023
AaronRobinsonMSFT
AaronRobinsonMSFT reviewed Feb 15, 2023
View reviewed changes
Comment threadsrc/mono/mono/metadata/gc-internals.h Outdated
BrzVlad
BrzVlad reviewed Feb 15, 2023
View reviewed changes
Comment threadsrc/mono/mono/sgen/sgen-gc.c Outdated
@BrzVlad

BrzVlad commented Feb 15, 2023

Copy link
Copy Markdown
Member

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

naricc commented Feb 15, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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-analysisbuild-analysisBot mentioned this pull request Feb 15, 2023
Closed
@naricc
nariccforce-pushed the naricc/add-memory-pressure branch from ea31496 to c677eaeCompareFebruary 15, 2023 22:07
lambdageek
lambdageek reviewed Feb 16, 2023
View reviewed changes
Comment threadsrc/mono/mono/sgen/sgen-gc.c Outdated
Comment threadsrc/mono/mono/sgen/sgen-gc.c Outdated
Comment threadsrc/mono/mono/sgen/sgen-gc.c Outdated
@naricc
nariccforce-pushed the naricc/add-memory-pressure branch from 12addc6 to f6235fdCompareFebruary 16, 2023 14:54
lambdageek
lambdageek reviewed Feb 16, 2023
View reviewed changes
Comment threadsrc/mono/mono/sgen/sgen-gc.c
@lambdageek

This comment was marked as outdated.

Sign in to view
@jonpryorjonpryor mentioned this pull request Feb 16, 2023
Closed
@build-analysisbuild-analysisBot mentioned this pull request Feb 16, 2023
Closed
@naricc
naricc merged commit 0d04df6 into dotnet:mainFeb 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
@jonpryorjonpryor 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
@ghostghost locked as resolved and limited conversation to collaborators Mar 20, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@BrzVladBrzVladBrzVlad left review comments
@AaronRobinsonMSFTAaronRobinsonMSFTAaronRobinsonMSFT left review comments
@lambdageeklambdageeklambdageek approved these changes
@vargazvargazAwaiting requested review from vargaz
@thaystgthaystgAwaiting requested review from thaystgthaystg is a code owner
@marek-safarmarek-safarAwaiting requested review from marek-safar

Assignees

@nariccnaricc

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