[core, ios, macos, android] Support non-copyable types with unique_any #10459

asheemmamoowala · 2017-11-14T00:27:16Z

Addresses @ivovandongen 's comment in #9983.

The problem highlighted is that the C++ Peer (mbgl::android::CustomGeometrySource) keeps a strong reference to the Java peer, creating a reference loop and preventing it from being garbage collected.

The solution is to inverse the ownership relationship between the Java Peer (JP), C++ Peer(CP), and core source (CS) after it is added to the map.

Prior to this change:

Before adding to the map:
JP owns CP (through jni::registerNativePeer helpers)
CP owns CS (through ownedSource unique_ptr)
After adding to the map:
JP owns CP (remains unchanged)
Core style owns CS (ownedSource is released)

After this change:

Before adding to the map:
JP has a shared pointer to CP (through shared_ptr managed by initialize and finalize lambdas)
CP has a weak global reference to JP
CP owns CS (through ownedSource unique_ptr)
CS has a shared pointer to CP (through the any peer field)
After adding to the map:
CP owns JP (through a strong global reference)
CS owns CP (through the shared_ptr in the peer field)
Core style owns the CS

CusotmGeometrySource needs a global reference to the java peer to call out into the JVM when the fetchTile and cancelTile callbacks are invoked in core. These methods are invoked on the render thread, while the java peer reference was originally created on the main thread.

cc @ivovandongen @jfirebaugh

jfirebaugh

This looks more complicated than it needs to be, particularly the use of shared_ptr / enabled_shared_from_this. I suggest the following scheme:

Not added to a style

No changes to the existing semantics:

Java peer has a raw, owning pointer to C++ peer.
C++ peer has a unique_ptr to C++ core source.

Avoid the need for the C++ peer to have a weak reference to the Java peer as follows:

The only place the weak reference is needed is in CustomGeometrySource::addToMap. If addToMap receives the Java peer as an argument, it doesn't need the weak reference.
So change NativeMapView::addSource to receive a jni::Object<Source> Java peer, rather than raw Source* as jni::jlong. Obtain the C++ peer from the Java peer in the usual way, then pass the Java peer as an additional argument to addToMap.

Avoid storing anything in the any field of the C++ core object at this stage. That isn't needed because there will be no calls to the callbacks of a source that's not added to a style.

Added to a style

Preserve the existing semantics:

Java peer has a raw pointer to C++ peer.
C++ peer has a reference to the C++ core source, but transfers ownership of the unique_ptr to the core style.

Add the following additional semantics:

The C++ peer gets a global reference to the Java peer (as in this PR currently).
Use the any field in the C++ core style to store a unique_ptr to the C++ peer. The core style now owns core source, and the core source owns the C++ peer. The Java peer still has a raw pointer to the C++ peer, but it is now considered to be non-owning. unique_ptr suffices to capture this ownership structure; shared_ptr isn't necessary.

This seems fairly simple to reason about. When a source is not in a style, ownership points one way:

Java peer → C++ peer → core source

When a source is in a style, it points the other way:

Java peer ← C++ peer ← core source ← core style

No shared ownership is needed.

Note that you do need to handle the case where a source is removed from the style, and re-reverse the ownership in that case. That doesn't appear to be included in this PR currently.

jfirebaugh · 2017-11-14T18:52:37Z

platform/android/src/style/sources/custom_geometry_source.cpp

@@ -17,11 +16,17 @@

 namespace mbgl {
 namespace android {
+    struct SourceWrapper {


SourceWrapper isn't necessary, you can store a std::shared_ptr<CustomGeometrySource> (or unique_ptr as I'm suggesting) directly in the any.

jfirebaugh · 2017-11-14T18:53:36Z

platform/android/src/style/sources/custom_geometry_source.cpp

+                             jni::Object<> options) {
+            auto sharedSource = std::make_shared<CustomGeometrySource>(env, javaSource, sourceId, options);
+            javaSource.Set(env, nativePtrField, reinterpret_cast<jni::jlong>(sharedSource.get()));
+            sharedSource->source.peer = SourceWrapper { sharedSource->shared_from_this() };


You don't need shared_from_this here -- you can just assign sharedSource->source.peer = sharedSource.

jfirebaugh · 2017-11-14T19:01:59Z

platform/android/src/style/sources/custom_geometry_source.cpp

+        auto finalize = [](jni::JNIEnv& env, jni::Object<CustomGeometrySource> javaSource) {
+            auto sharedSource = std::shared_ptr<CustomGeometrySource>(reinterpret_cast<CustomGeometrySource *>((jlong)javaSource.Get(env, nativePtrField)));
+            if (sharedSource) javaSource.Set(env, nativePtrField, (jlong)0);
+            sharedSource.reset();


This resets only the local instance of shared_ptr, not the one in the C++ core peer field, so both the C++ peer and the C++ core object will leak when a CustomGeometrySource that's not added to a map is finalized.

jfirebaugh · 2017-11-14T19:07:30Z

platform/android/src/style/sources/custom_geometry_source.cpp

+        };
+
+        auto finalize = [](jni::JNIEnv& env, jni::Object<CustomGeometrySource> javaSource) {
+            auto sharedSource = std::shared_ptr<CustomGeometrySource>(reinterpret_cast<CustomGeometrySource *>((jlong)javaSource.Get(env, nativePtrField)));


Register "finalize" with the RegisterNativePeer call so that you don't need to manually lookup the field and cast.

asheemmamoowala · 2017-11-17T03:27:22Z

any requires that contained types be copy-constructible. This eliminates the possibility of using unique_ptr<T> directly or wrapped in some simple object.

Instead of trying to hack around the copy-constructability requirement of any, it made sense to implement a unique_any type that relaxes can work with non-copyable types. It does not support copy construction or copy assignment.

I also pushed these changes down to mbgl::android::Source so they ownership behavior is available to all source types. This has the additional benefit of being able to reuse java source objects in calls to MapView.GetSource().

The ownership changes have been implemented with unique_any :

Before the source has been added to a style:

Java peer → C++ peer → core source

When a source is in a style, it points the other way:

Java peer ← C++ peer ← core source ← core style

In addition, when a source is removed from the map, the original ownership chain is restored.

jfirebaugh · 2017-11-17T21:20:36Z

platform/darwin/src/MGLStyle.mm

@@ -221,7 +221,7 @@ - (MGLSource *)sourceWithIdentifier:(NSString *)identifier
 }

 - (MGLSource *)sourceFromMBGLSource:(mbgl::style::Source *)rawSource {
-    if (MGLSource *source = rawSource->peer.empty() ? nil : mbgl::any_cast<SourceWrapper>(rawSource->peer).source) {
+    if (MGLSource *source = rawSource->peer.has_value() ?  mbgl::util::any_cast<SourceWrapper>(&(rawSource->peer))->source : nil) {


Can this continue to use the throwing version of any_cast rather than the null-returning version, so that if it fails it's not undefined behavior?

jfirebaugh · 2017-11-17T21:22:45Z

platform/android/src/style/sources/sources.cpp

@@ -20,6 +20,11 @@ namespace {

    Source* initializeSourcePeer(mbgl::style::Source& coreSource) {
        Source* source;
+        if (coreSource.peer.has_value()) {
+            source = mbgl::util::any_cast<std::unique_ptr<Source>>(&coreSource.peer)->get();
+            return source;


Nit: follow the existing convention here -- change the subsequent if to else if and omit the early return -- or convert all the branches to early return and eliminate the source temporary variably entirely.

jfirebaugh · 2017-11-17T21:28:34Z

platform/android/src/style/sources/source.cpp

@@ -68,6 +107,14 @@ namespace android {
        return std::move(ownedSource);
    }

+    jni::jobject* Source::getJavaPeer(jni::JNIEnv& env) {


Is this called anywhere?

That's a 🐛 . It should've been called in createJavaSourcePeer()

ivovandongen

Looks good, minor nits.

I wonder if we should let the c++ peer hold onto the (weak) reference of the Java peer eventually instead of passing it along in the native mapview methods. But that can be taken care of lateron.

ivovandongen · 2017-11-21T14:40:59Z

platform/android/src/native_map_view.cpp

@@ -858,7 +858,6 @@ jni::Array<jni::Object<Source>> NativeMapView::getSources(JNIEnv& env) {
    for (auto source : sources) {
        auto jSource = jni::Object<Source>(createJavaSourcePeer(env, *rendererFrontend, *source));
        jSources.Set(env, index, jSource);
-        jni::DeleteLocalRef(env, jSource);


Should this actually be removed? jSource is still a local reference here right? In practice there will not be that many sources, but this could potentially overflow the local reference table I think.

This is now a pointer to the global reference from GetJavaPeer.

ivovandongen · 2017-11-21T14:41:27Z

platform/android/src/style/sources/source.cpp

+    }
+
+    void Source::removeFromMap(JNIEnv&, jni::Object<Source>, mbgl::Map& map) {
+        //Cannot remove if not attached yet


ivovandongen · 2017-11-21T14:41:49Z

platform/android/src/style/sources/source.cpp

@@ -68,6 +112,16 @@ namespace android {
        return std::move(ownedSource);
    }

+    jni::jobject* Source::getJavaPeer(jni::JNIEnv& env) {
+        if(!javaPeer) {


ivovandongen · 2017-11-21T14:42:45Z

platform/android/src/style/sources/source.cpp

+        //  while also resetting it's nativePtr to 0 to prevent the subsequent GC of the Java peer from
+        // re-entering this dtor.
+        if (ownedSource.get() == nullptr && javaPeer.get() != nullptr) {
+            //Manually clear the java peer


ivovandongen · 2017-11-21T14:44:53Z

platform/darwin/src/MGLStyle.mm

@@ -221,7 +221,7 @@ - (MGLSource *)sourceWithIdentifier:(NSString *)identifier
 }

 - (MGLSource *)sourceFromMBGLSource:(mbgl::style::Source *)rawSource {
-    if (MGLSource *source = rawSource->peer.empty() ? nil : mbgl::any_cast<SourceWrapper>(rawSource->peer).source) {
+    if (MGLSource *source = rawSource->peer.has_value() ?  mbgl::util::any_cast<SourceWrapper>(rawSource->peer).source : nil) {


extra space after ?

jfirebaugh · 2017-11-21T17:24:22Z

platform/android/src/style/sources/sources.cpp

    peerSource.release();
    return result;
 }

+jni::jobject* removeSourceFromMap(std::unique_ptr<mbgl::style::Source>&& coreSource) {


This method is confusing. Why is it named removeSourceFromMap, when it doesn't do that? Why does it construct a unique pointer and then immediate release it?

unique_ptr usage is not necessary and can be removed from both this function and createJavaSourcePeer.

jfirebaugh · 2017-11-21T17:31:07Z

platform/android/src/style/sources/sources.cpp

@@ -43,11 +45,16 @@ namespace android {
 jni::jobject* createJavaSourcePeer(jni::JNIEnv& env, AndroidRendererFrontend& frontend, mbgl::style::Source& coreSource) {
    std::unique_ptr<Source> peerSource = std::unique_ptr<Source>(initializeSourcePeer(coreSource));


This doesn't look right. Why construct a unique pointer from a raw pointer, which might be owned by some other unique_ptr, and then release it later?

jfirebaugh · 2017-11-21T17:41:13Z

platform/android/src/style/sources/sources.cpp

@@ -20,7 +20,9 @@ namespace {

    Source* initializeSourcePeer(mbgl::style::Source& coreSource) {
        Source* source;
-        if (coreSource.is<mbgl::style::VectorSource>()) {
+        if (coreSource.peer.has_value()) {


I think this case should be checked by createJavaSourcePeer, not by initializeSourcePeer. The return type of initializeSourcePeer should be std::unique_ptr<Source>.

initializeSourcePeer isn't just an initializer anymore. I think it can be changed to getSourcePeer and skip the unique_ptr<> since it's not really needed in createJavaSourcePeer or removeSourceFromMap

jfirebaugh · 2017-11-22T00:53:08Z

platform/android/src/native_map_view.cpp

@@ -856,9 +856,8 @@ jni::Array<jni::Object<Source>> NativeMapView::getSources(JNIEnv& env) {
    jni::Array<jni::Object<Source>> jSources = jni::Array<jni::Object<Source>>::New(env, sources.size(), Source::javaClass);
    int index = 0;
    for (auto source : sources) {
-        auto jSource = jni::Object<Source>(createJavaSourcePeer(env, *rendererFrontend, *source));
+        auto jSource = jni::Object<Source>(Source::peerForCoreSource(env, *source, *rendererFrontend));


Eliminate this temporary variable.

jfirebaugh · 2017-11-22T00:53:24Z

platform/android/src/native_map_view.cpp

@@ -874,38 +873,25 @@ jni::Object<Source> NativeMapView::getSource(JNIEnv& env, jni::String sourceId)
    }

    // Create and return the source's native peer
-    return jni::Object<Source>(createJavaSourcePeer(env, *rendererFrontend, *coreSource));
+    return jni::Object<Source>(Source::peerForCoreSource(env, *coreSource, *rendererFrontend));


jni::Object<Source> isn't necessary.

jfirebaugh · 2017-11-22T00:56:43Z

platform/android/src/style/sources/source.hpp

     */
-    Source(mbgl::style::Source&);
+    Source(jni::JNIEnv&, mbgl::style::Source&, jni::jobject*, AndroidRendererFrontend&);


jni::jobject* → jni::Object<Source>

jfirebaugh · 2017-11-22T00:57:51Z

platform/android/src/style/sources/source.cpp

+        // Remove the source from the map and take ownership
+        ownedSource = map.getStyle().removeSource(source.getID());
+
+        // The source may not be removed if any layers still reference it


Good catch.

…ce in the C++ peer.

asheemmamoowala added the Android Mapbox Maps SDK for Android label Nov 14, 2017

asheemmamoowala requested review from ivovandongen and jfirebaugh November 14, 2017 00:27

jfirebaugh suggested changes Nov 14, 2017

View reviewed changes

asheemmamoowala force-pushed the android-custom-source-leak branch from ef61709 to abd6289 Compare November 17, 2017 03:17

asheemmamoowala changed the title ~~[android] Use shared_ptr<> with CustomGeometrySource for Native peer~~ [core, ios, macos, android] Support non-copyable types with unique_any Nov 17, 2017

asheemmamoowala force-pushed the android-custom-source-leak branch from abd6289 to 956e013 Compare November 17, 2017 21:11

jfirebaugh reviewed Nov 17, 2017

View reviewed changes

asheemmamoowala force-pushed the android-custom-source-leak branch 2 times, most recently from 0d157be to 379baa3 Compare November 18, 2017 01:04

asheemmamoowala force-pushed the ahm-custom-vector-source branch 2 times, most recently from 6ec1ec3 to 7d28ab7 Compare November 20, 2017 23:16

asheemmamoowala force-pushed the android-custom-source-leak branch from 379baa3 to 9e70ea0 Compare November 21, 2017 08:15

ivovandongen approved these changes Nov 21, 2017

View reviewed changes

jfirebaugh suggested changes Nov 21, 2017

View reviewed changes

jfirebaugh reviewed Nov 21, 2017

View reviewed changes

[core, ios, macos] Implement unique_any and remove linb::any

321e6eb

asheemmamoowala force-pushed the android-custom-source-leak branch 2 times, most recently from 4399576 to 75b84ae Compare November 22, 2017 00:13

jfirebaugh reviewed Nov 22, 2017

View reviewed changes

Asheem Mamoowala added 2 commits November 21, 2017 17:25

[android] Reuse Java Source objects by holding on to a strong referen…

4e73963

…ce in the C++ peer.

[android] Use inherited javaPeer in CustomGeometrySource C++ peer

ccc57ae

asheemmamoowala force-pushed the android-custom-source-leak branch from 75b84ae to ccc57ae Compare November 22, 2017 01:25

jfirebaugh approved these changes Nov 22, 2017

View reviewed changes

asheemmamoowala merged this pull request into ahm-custom-vector-source Nov 22, 2017

asheemmamoowala deleted the android-custom-source-leak branch November 22, 2017 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core, ios, macos, android] Support non-copyable types with unique_any #10459

[core, ios, macos, android] Support non-copyable types with unique_any #10459

asheemmamoowala commented Nov 14, 2017

jfirebaugh left a comment •

edited

Loading

jfirebaugh Nov 14, 2017

jfirebaugh Nov 14, 2017

jfirebaugh Nov 14, 2017

jfirebaugh Nov 14, 2017

asheemmamoowala commented Nov 17, 2017 •

edited

Loading

jfirebaugh Nov 17, 2017

jfirebaugh Nov 17, 2017

jfirebaugh Nov 17, 2017

asheemmamoowala Nov 18, 2017

ivovandongen left a comment

ivovandongen Nov 21, 2017

asheemmamoowala Nov 21, 2017

ivovandongen Nov 21, 2017

ivovandongen Nov 21, 2017

ivovandongen Nov 21, 2017

ivovandongen Nov 21, 2017

jfirebaugh Nov 21, 2017

asheemmamoowala Nov 21, 2017

jfirebaugh Nov 21, 2017

jfirebaugh Nov 21, 2017

asheemmamoowala Nov 21, 2017

jfirebaugh Nov 22, 2017

jfirebaugh Nov 22, 2017

jfirebaugh Nov 22, 2017

jfirebaugh Nov 22, 2017

		@@ -43,11 +45,16 @@ namespace android {
		jni::jobject* createJavaSourcePeer(jni::JNIEnv& env, AndroidRendererFrontend& frontend, mbgl::style::Source& coreSource) {
		std::unique_ptr<Source> peerSource = std::unique_ptr<Source>(initializeSourcePeer(coreSource));

[core, ios, macos, android] Support non-copyable types with unique_any #10459

[core, ios, macos, android] Support non-copyable types with unique_any #10459

Conversation

asheemmamoowala commented Nov 14, 2017

jfirebaugh left a comment • edited Loading

Choose a reason for hiding this comment

Not added to a style

Added to a style

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asheemmamoowala commented Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivovandongen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfirebaugh left a comment •

edited

Loading

asheemmamoowala commented Nov 17, 2017 •

edited

Loading