Allow MonoGCBridgeSCC objects to have an empty User Peer list#154
Merged
Conversation
Mono passes gc_cross_references a description of a graph. Each node in the graph has a list of User Peer objects which should be collected together in the upcoming GC. This list is required by both the spec and the current code to be nonempty. To enable new optimizations, the runtime team wants to modify the spec to allow the list to be empty. This patch makes that possible by creating a new temporary object of type GCUserPeer when a MonoGCBridgeSCC with an empty list is seen.
Collaborator
|
Hello! I'm the build bot for the Mono project. I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done. Contributors can ignore this message. |
| java_class = (*env)->GetObjectClass (env, handle); | ||
| add_method_id = (*env)->GetMethodID (env, java_class, "monodroidAddReference", "(Ljava/lang/Object;)V"); | ||
| if (add_method_id) { | ||
| mono.mono_field_get_value (reffed_obj, bridge_info->handle, &reffed_handle); |
Contributor
Author
There was a problem hiding this comment.
Notice there is a pretty big bug here which this patch fixes. The same bridge_info is used for obj and reffed_obj. If obj is an Object and reffed_obj is a Throwable, this could lead to Something Very Bad happening.
Instead of loading java.util.ArrayList and mono.android.GCUserPeer every GC, load them only once and save GREFs indefinitely.
In the GC bridge handling, we need to be able to map bridgeless SCCs to their index in the temporary_peers array. Currently we store the indices in a lookaside table. This patch instead hides the index in a field in MonoGCBridgeSCC which is unused for bridgeless SCCs, allowing us to avoid allocating the lookaside.
xmcclure
force-pushed
the
bridgeless-scc-support
branch
from
048b0d9 to
8fabd8b
Compare
| @@ -0,0 +1,19 @@ | |||
| package mono.android; | |||
|
|
|||
| class GCUserPeer { | |||
Contributor
There was a problem hiding this comment.
shouldn't this implement IGCUserPeer?
Contributor
Author
There was a problem hiding this comment.
It could, it doesn't need to. We load monodroidAddReference off of the class directly using GetMethodID. In fact, monodroid-glue doesn't use IGCUserPeer at all. We fetch a gref to it at system start but then never do anything with that gref.
Would you prefer I have GCUserPeer implement IGCUserPeer?
xmcclure
force-pushed
the
bridgeless-scc-support
branch
from
6b1ef47 to
230782e
Compare
|
|
||
| // These will be loaded as needed and persist between GCs | ||
| // FIXME: This code assumes it is totally safe to hold onto these GREFs forever. Can mono.android.jar ever be unloaded? | ||
| static jobject jni_arraylist, jni_gcuserpeer; |
Contributor
There was a problem hiding this comment.
Convention is:
- Prefix based on the Java class name (excluding package), preserving case. Good:
ArrayList. Bad:arraylist. - Underscore.
- "Member name": the field/method name, or
ctorfor constructors, orclassforjclassvalues.
These are also usually done one per line, organized by type:
Thus:
static jclass ArrayList_class;
static jmethodID ArrayList_ctor;
static jmethodID ArrayList_get;
static jmethodID ArrayList_add;
static jclass GCUserPeer_class;
static jmethodID GCUserPeer_ctor;
…dex stash" outlive prepare function
xmcclure
changed the title
| { | ||
| if (!target.is_mono_object) | ||
| (*env)->DeleteLocalRef (env, target.jobj); | ||
| } |
Contributor
There was a problem hiding this comment.
Should we set target.jobj=NULL?
Contributor
Author
There was a problem hiding this comment.
Hm, I do not think so, any more than DeleteLocalRef should set it to NULL. This function is only used when the target object is done with (and currently is set in only one well-defined place).
Contributor
Author
There was a problem hiding this comment.
Wait, hold on: No, we should not set it to NULL, because target is not being passed by reference. "target" is here a local variable of target_release.
monojenkins
added a commit
to mono/mono
that referenced
this pull request
Aug 29, 2016
Make GC bridge perform well in the "double fan" scenario The GC bridge currently performs VERY poorly when the object graph has a "double fan" shape. If M Java objects point to 1 C# object which points to N Java objects, the graph exported to Monodroid will currently drop the C# object node and replace it with (M\*N) edges. It is easy to write code where (M\*N) grows ludicrously big. In testing on device, this patch solves that problem while leaving other cases unaffected. In testing on device, I found: * All performance tests except double fan: No discernible performance difference after patch * Double fan test with M=N=1000: Before the patch this took between three and seven seconds. After the patch this took between 0.2 and 0.3 seconds. * Double fan test with M=N=4000: Before the patch this took anywhere from one to two minutes. After the patch, this took about 1.2 seconds. * Double fan test with M=N=6000: Before the patch, this took so long I was not able to get it to ever complete. After the patch, this took about 1.3 seconds. * Double fan test with M=N=20000: I did not attempt this test pre-patch. After the patch, it took about 5.6 seconds. The commit messages describe the changes in some detail but the short version is: * Add a mechanism for exporting non-bridged SCCs to the bridge client * Turn that mechanism on whenever the product fanin*fanout grows too large * Make the merge cache more aggressive To function, this patch requires a corresponding change to monodroid. See dotnet/android#154
radical
pushed a commit
that referenced
this pull request
May 8, 2018
…#154) Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=56537 If a class derives from `Java.Lang.Object` it should have the two properties mentioned above generated, so that it is possible to find methods in the correct instance of Java class instance that is wrapped by our managed class. Generator used to decide whether or not to generate these properties based on the presence of class members (fields, constructors, methods and properties) or whether the class is an annotation in the API description file. However there exist a number of classes (for instance `Inet4Address`) which don't have any of the members present and yet they should have the Threshold* properties generated for the reasons described above. The `HasClassHandle` property which was used to determine whether the class should get these properties (among a handful of other members) doesn't serve its purpose correctly leading to corner cases when the `Threshold*` properties are missing and causing runtime bugs (e.g. calling `GetAddress()` on an instance of `Inet4Address` returns `null` even though the underlying Java class has the information - the call never reaches `Inet4Address` instance and thus returns nothing). Replacing `HasClassHandle` with a simple check for whether the class in question derives from `Java.Lang.Object` is the correct fix that ensures the missing properties are generated.
picenka21
pushed a commit
to picenka21/runtime
that referenced
this pull request
Feb 18, 2022
Make GC bridge perform well in the "double fan" scenario The GC bridge currently performs VERY poorly when the object graph has a "double fan" shape. If M Java objects point to 1 C# object which points to N Java objects, the graph exported to Monodroid will currently drop the C# object node and replace it with (M\*N) edges. It is easy to write code where (M\*N) grows ludicrously big. In testing on device, this patch solves that problem while leaving other cases unaffected. In testing on device, I found: * All performance tests except double fan: No discernible performance difference after patch * Double fan test with M=N=1000: Before the patch this took between three and seven seconds. After the patch this took between 0.2 and 0.3 seconds. * Double fan test with M=N=4000: Before the patch this took anywhere from one to two minutes. After the patch, this took about 1.2 seconds. * Double fan test with M=N=6000: Before the patch, this took so long I was not able to get it to ever complete. After the patch, this took about 1.3 seconds. * Double fan test with M=N=20000: I did not attempt this test pre-patch. After the patch, it took about 5.6 seconds. The commit messages describe the changes in some detail but the short version is: * Add a mechanism for exporting non-bridged SCCs to the bridge client * Turn that mechanism on whenever the product fanin*fanout grows too large * Make the merge cache more aggressive To function, this patch requires a corresponding change to monodroid. See dotnet/android#154 Commit migrated from mono/mono@66fd6fd
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mono passes gc_cross_references a description of a graph. Each node in the graph has a list of User Peer objects which should be collected together in the upcoming GC. This list is required by both the spec and the current code to be nonempty. To enable new optimizations, the runtime team wants to modify the spec to allow the list to be empty. This patch makes that possible by creating a new temporary object of type GCUserPeer when a MonoGCBridgeSCC with an empty list is seen.
I think this patch should not be merged until tests with Mono have shown that this can actually produce a performance improvement. In the meantime you can test this with https://github.com/xmcclure/mono/tree/andi-phantom-test-1 and https://github.com/xmcclure/monodroid/commits/andi-phantom-test-1 .