Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Runtime polymorphic conversion #10075

Merged
merged 1 commit into from
Oct 23, 2017
Merged

Conversation

jfirebaugh
Copy link
Contributor

@jfirebaugh jfirebaugh commented Sep 26, 2017

  • Fix CI
  • Decide on alternate name for Value
  • Explanatory comments
  • Choose the size of the std::aligned_storage based on environment: we know which type is used for each platform, so we can use max(sizeof(JSValue*), sizeof(PLATFORM_VALUE_TYPE))
  • Diff includes a gradle version update (2.3.1 → 2.3.3). Should probably be reverted.
  • make_property_setters.hpp.ejs needs to be updated
  • Final bloaty check

First pass at a bit of binary size reduction by reducing templating in the mbgl::style::conversions namespace.

On macOS framework:

     VM SIZE                               FILE SIZE
 ++++++++++++++ GROWING                 ++++++++++++++
  +0.5%    +976 __TEXT,__const             +976  +0.5%
  +0.9%    +272 __DATA,__const             +272  +0.9%
  +0.8%    +160 __DATA,__data              +160  +0.8%
  +4.3%     +96 __DATA,__bss                  0  [ = ]

 -------------- SHRINKING               --------------
  -5.7%  -165Ki __TEXT,__text            -165Ki  -5.7%
  -3.7% -13.8Ki __TEXT,__gcc_except_tab -13.8Ki  -3.7%
  -0.1%    -228 [None]                  -2.79Ki  -1.5%
  -3.2% -2.42Ki __TEXT,__unwind_info    -2.42Ki  -3.2%

  -4.4%  -180Ki TOTAL                    -182Ki  -4.5%

cc @kkaefer @anandthakker


// https://github.com/mapbox/mapbox-gl-native/issues/5623
optional<GeoJSON> (*toGeoJSON) (const Storage&, Error&);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you decide to implement this as an explicit data structure vs. relying on C++' built-in virtual call system?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it doesn't require heap allocation and using unique_ptr everywhere.

struct Error { std::string message; };

class Value {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not feeling great about introducing another class called Value. Is there a different name that would fit too?

Copy link
Contributor

Choose a reason for hiding this comment

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

JSONValue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Convertee?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going with Convertible, as suggested by @1ec5.

@kkaefer
Copy link
Contributor

kkaefer commented Sep 27, 2017

Also, amazing size reductions!

@anandthakker
Copy link
Contributor

@jfirebaugh @kkaefer do you feel good about going ahead with this approach? I'd like to rebase the expressions branch onto it to see how that looks, but in order to do so I'll need to implement this new system for node (v8::Value) conversion. (While we're at it, we will also either need to solve
or workaround #5623, since this PR removes convert<GeoJSON>(string ,error))

@kkaefer kkaefer added the Core The cross-platform C++ core, aka mbgl label Oct 11, 2017
@anandthakker
Copy link
Contributor

anandthakker commented Oct 14, 2017

Some additions in #10184:

  • Refactors this to centralize all the VTable stuff in one place, so that each platform's implementation only has to deal with the actual conversion methods (as before)
  • Updates android conversion

TBD:

  • Update Qt conversion
  • Update ios conversion
  • Choose the size of the std::aligned_storage based on environment: we know which type is used for each platform, so we can use max(sizeof(JSValue*), sizeof(PLATFORM_VALUE_TYPE))

@anandthakker
Copy link
Contributor

anandthakker commented Oct 14, 2017

@jfirebaugh @kkaefer I went ahead and merged in the additions from #10184 (it's linear, so we can always revert, but seems easier to review in one place)

The main remaining item other than getting & responding to a review is to set the size of the size of the std::aligned_storage based on the platform. Does our build setup afford an easy way to do that?

Also @jfirebaugh re: @kkaefer 's point above about the name Value, how do you feel about JSONValue?

@anandthakker
Copy link
Contributor

Rebased onto master to resolve a conflict; previous version of "Move conversions to cpp files where possible" is at cd7e4c2

@anandthakker anandthakker force-pushed the runtime-polymorphic-conversion branch 2 times, most recently from 800c095 to 18f9fc6 Compare October 16, 2017 17:11
Copy link
Contributor Author

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

package-lock.json got added accidentally.

}

template <typename T>
void destroy (detail::Storage& storage) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The inverse of placement new is calling the destructor manually, so this can be written:

template <typename T>
void destroy (detail::Storage& storage) {
    cast<T>(storage).~T();
}

Then inline it.

}

template <typename T>
void move(detail::Storage&& src, detail::Storage& dest) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inline this too.

*/

namespace detail {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can Storage nest in Value instead?

struct Error { std::string message; };

template <typename T>
const T& cast(const detail::Storage& storage) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If reinterpret_casts work, just use them inline.

const v8::Local<v8::Value> value(std::move(cast(storage)));
(void)value; // appease linter
}
using V8Value = v8::Local<v8::Value>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure this typedef adds much value. I'd rather just use v8::Local<v8::Value> directly.

new (static_cast<void*>(&dest)) JSValue* (std::move(*static_cast<JSValue**>(static_cast<void*>(&src))));
destroy(src);
}
using JSValuePointer = const JSValue*;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same.

@anandthakker
Copy link
Contributor

@jfirebaugh updated to address review comments and get CI passing (I'll have to look at the one failing android build tomorrow). Questions:

  1. Shall I revert 2850f88?
  2. You okay with 4bd2a04? (The problem it's solving is that GCC considers the enum member mbgl::SourceType::GeoJSON to be shadowing the type mbgl::GeoJSON from util/geojson.cpp. They're both included (indirectly) by conversion/geojson_options.cpp.)

@jfirebaugh
Copy link
Contributor Author

CI is crashing on Android:

signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr --------
Stack frame #00 pc 0003b0ec  /system/lib/libc.so (tgkill+12)
Stack frame #01 pc 000173c1  /system/lib/libc.so (pthread_kill+52)
Stack frame #02 pc 00017fd3  /system/lib/libc.so (raise+10)
Stack frame #03 pc 00014795  /system/lib/libc.so (__libc_android_abort+36)
Stack frame #04 pc 00012f44  /system/lib/libc.so (abort+4)
Stack frame #05 pc 00228cd7  /system/lib/libart.so (art::Runtime::Abort()+170)
Stack frame #06 pc 000a7371  /system/lib/libart.so (art::LogMessage::~LogMessage()+1360)
Stack frame #07 pc 000b1b17  /system/lib/libart.so (art::JniAbort(char const*, char const*)+1118)
Stack frame #08 pc 000b2055  /system/lib/libart.so (art::JniAbortF(char const*, char const*, ...)+68)
Stack frame #09 pc 000b453b  /system/lib/libart.so (_ZN3art11ScopedCheck5CheckEbPKcz.constprop.129+710)
Stack frame #10 pc 000b76eb  /system/lib/libart.so (art::CheckJNI::IsInstanceOf(_JNIEnv*, _jobject*, _jclass*)+50)
Stack frame #11 pc 00109d4d  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so
Stack frame #12 pc 00109ca1  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so
Stack frame #13 pc 00128e1b  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so
Stack frame #14 pc 0008e93d  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so
Stack frame #15 pc 0008e919  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so
Stack frame #16 pc 0008e8f5  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so
Stack frame #17 pc 0008d2b5  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so
Stack frame #18 pc 00317a57  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so
Stack frame #19 pc 003178ad  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so
Stack frame #20 pc 003176b1  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so
Stack frame #21 pc 003174fd  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so
Stack frame #22 pc 00317455  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so
Stack frame #23 pc 00365c0b  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so
Stack frame #24 pc 002f8953  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so
Stack frame #25 pc 0014ecbb  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so
Stack frame #26 pc 00151bb3  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so
Stack frame #27 pc 00151b11  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so
Stack frame #28 pc 00151aab  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so
Stack frame #29 pc 00151c7d  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so
Stack frame #30 pc 00151c13  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so
Stack frame #31 pc 0019b1bf  /data/dalvik-cache/arm/data@app@com.mapbox.mapboxsdk.testapp-1@base.apk@classes.dex

@anandthakker Do you remember seeing this before or is it something I introduced with the latest changes?

@anandthakker
Copy link
Contributor

anandthakker commented Oct 18, 2017 via email

@jfirebaugh jfirebaugh force-pushed the runtime-polymorphic-conversion branch 3 times, most recently from fe786a1 to 5193d5b Compare October 18, 2017 19:35
@jfirebaugh
Copy link
Contributor Author

I'm able to reproduce the crash locally. I believe it's due to mbgl::android::Value prematurely deleting a local ref, and the solution will be to integrate the latest jni.hpp and replace mbgl::android::Value with jni::UniqueLocalRef.

@jfirebaugh
Copy link
Contributor Author

     VM SIZE                               FILE SIZE
 ++++++++++++++ GROWING                 ++++++++++++++
  +0.5%    +992 __TEXT,__const             +992  +0.5%
  +1.0%    +272 __DATA,__const             +272  +1.0%
  +0.9%    +176 __DATA,__data              +176  +0.9%
  +0.1%    +166 __TEXT,__cstring           +166  +0.1%
  +4.3%     +96 __DATA,__bss                  0  [ = ]

 -------------- SHRINKING               --------------
  -5.1%  -147Ki __TEXT,__text            -147Ki  -5.1%
  -2.9% -11.0Ki __TEXT,__gcc_except_tab -11.0Ki  -2.9%
  -0.3%    -498 [None]                  -2.83Ki  -1.5%
  -3.3% -2.52Ki __TEXT,__unwind_info    -2.52Ki  -3.3%

  -3.9%  -160Ki TOTAL                    -162Ki  -4.0%

Copy link
Contributor

@anandthakker anandthakker left a comment

Choose a reason for hiding this comment

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

🎉

@jfirebaugh LGTM, with the caveat that I don't fully understand why the switch from const refs to moves solves the android problem

// Qt: JSValue* or QVariant

// TODO: use platform-specific size
using Storage = std::aligned_storage_t<32, 8>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noting that when I changed this from 16 to 32, I didn't actually confirm that 32 was the minimum sufficient size, because I thought we'd be able to just use sizeof(platform_specific_type) when we did the platform-specific sizing.

Value(jni::JNIEnv&, jni::jobject*);
virtual ~Value();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was the virtual destructor here in the first place? Was Value used as a base class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there was any reason for it.

@jfirebaugh jfirebaugh force-pushed the runtime-polymorphic-conversion branch from 1e53e9a to 4e6c498 Compare October 23, 2017 16:14
@jfirebaugh jfirebaugh merged commit 4b2e1cd into master Oct 23, 2017
@jfirebaugh jfirebaugh deleted the runtime-polymorphic-conversion branch October 23, 2017 16:56
@marcovc
Copy link

marcovc commented Nov 9, 2017

Hi,
I'm getting compilation errors using g++6.3 and seems related to this issue. Which compiler are you using?
Thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants