Remove kotlin-reflect, replace with kotlinx-metadata-jvm #1076

ZacSweers · 2020-01-15T07:53:39Z

Kotlin-reflect has been a long-standing thorn for KotlinJsonAdapter for a number of reasons:

It has a huge impact on binary size, both in terms of the library and the compiler impact. When removing it from the slack code base recently, it saved us nearly 10k methods just by removing it (even though only 2 were used!)
It's super slow and heavyweight
It has a number of tricky proguard considerations

This PR replaces kotlin-reflect entirely with kotlinx-metadata-reflect, the low-level metadata library we use in code gen (via kotlinpoet-metadata). It's also what the kotlinx.reflect.lite project is in the process of being migrated to, and used in a number of other major toolchains now (Dagger, R8, etc).

Effectively, this skips past kotlinx.reflect.lite and replaces kotlin-reflect with something closer to an "ultralight" setup, set up specific to Moshi's needs. It's a significant change to the adapter, but the core logic remains the same with one notable exception (documented at the end).

This supersedes #307, but effectively resolves #307 in spirit.

Stats

Quick androidx-benchmark on my serialization benchmarks project shows this is ~21.5% faster for buffer reads as well! ZacSweers/json-serialization-benchmarking#10

AndroidBenchmark.moshi_kotlin_codegen_buffer_toJson[minified=true]                   5,052,032  ns
AndroidBenchmark.moshi_kotlin_codegen_string_toJson[minified=true]                   7,072,449  ns
AndroidBenchmark.moshi_kotlin_reflective_metadata_buffer_toJson[minified=true]       8,096,823  ns
AndroidBenchmark.moshi_kotlin_reflective_buffer_toJson[minified=true]                9,461,355  ns
AndroidBenchmark.moshi_kotlin_reflective_string_toJson[minified=true]               11,308,542  ns

AndroidBenchmark.moshi_kotlin_codegen_buffer_fromJson[minified=true]                 7,553,282  ns
AndroidBenchmark.moshi_kotlin_codegen_string_fromJson[minified=true]                 8,746,668  ns
AndroidBenchmark.moshi_kotlin_reflective_metadata_buffer_fromJson[minified=true]     9,167,501  ns
AndroidBenchmark.moshi_kotlin_codegen_buffer_fromJson[minified=false]               10,039,950  ns
AndroidBenchmark.moshi_kotlin_reflective_buffer_fromJson[minified=true]             10,454,168  ns
AndroidBenchmark.moshi_kotlin_reflective_metadata_buffer_fromJson[minified=false]   11,691,721  ns
AndroidBenchmark.moshi_kotlin_codegen_string_fromJson[minified=false]               11,716,408  ns
AndroidBenchmark.moshi_kotlin_reflective_string_fromJson[minified=true]             11,734,532  ns
AndroidBenchmark.moshi_kotlin_reflective_buffer_fromJson[minified=false]            12,930,158  ns
AndroidBenchmark.moshi_kotlin_reflective_string_fromJson[minified=false]            14,747,397  ns

Two open questions

The behavior change is that moshi-kotlin still allowed extending from and encoding non-kotlin superclasses and their properties. I think this was an oversight when we stopped supporting mixing them in Moshi 1.8, as this doesn't match the behavior of code gen.
~~What are the appropriate proguard rules for this?~~

It's used there

ZacSweers · 2020-01-15T09:22:05Z

kotlin/reflect/src/main/java/com/squareup/moshi/kotlin/reflect/KotlinJsonAdapter.kt

+    for (property in allPropertiesSequence.distinctBy { it.name }) {
+      val propertyField = property.fieldSignature?.let { signature ->
+        val signatureString = signature.asString()
+        rawType.declaredFields.find { it.jvmFieldSignature == signatureString }


I think I need to do an allFields() style function for this as well to be safe.

ZacSweers · 2020-01-15T09:23:18Z

kotlin/reflect/src/main/java/com/squareup/moshi/kotlin/reflect/KotlinJsonAdapter.kt

+  return classMetadata.toKmClass()
+}
+
+private fun Class<*>.allMethods(): Sequence<Method> {


I wonder if this is worth sticking a lazily-filled cache in front of

rharter

This all sounds good to me. I haven't used these libraries extensively so I'm not sure I have the expertise to thoroughly evaluate the implementation, but don't see any red flags on review.

udalov · 2020-01-15T15:39:31Z

kotlin/reflect/src/main/java/com/squareup/moshi/kotlin/reflect/JvmDescriptorUtil.kt

+  get() {
+    return buildString {
+      append('(')
+      parameterTypes.joinTo(this, separator = "", transform = { it.descriptor })


Minor, but you can use one-liners for this

internal val Constructor.descriptor: String get() = parameterTypes.joinToString(separator = "", prefix = "(", postfix = ")V") { it.descriptor }

Good call! 6cff563

kotlin/reflect/src/main/java/com/squareup/moshi/kotlin/reflect/KotlinJsonAdapter.kt

udalov · 2020-01-15T15:47:47Z

kotlin/reflect/src/main/java/com/squareup/moshi/kotlin/reflect/KotlinJsonAdapter.kt

+    return when {
+      this === other -> true
+      this != null && other != null -> {
+        abbreviatedType == other.abbreviatedType


Note that this will imply that types which are represented with the same runtime type are different, if they have a different abbreviation. Not sure if you'd like this, but if you would, then never mind:

typealias ListOfString = List<String> typealias MyList<T> = List<T>

KmType of ListOfString isEqualTo KmType of MyList<String> -> false

Hmm, that's a good point. Since we're just doing this for checking equality of the parameter and property. I guess since it's a legal assignment we can drop the alias check. Done in 36f945b

udalov · 2020-01-15T15:48:53Z

kotlin/reflect/src/main/java/com/squareup/moshi/kotlin/reflect/KotlinJsonAdapter.kt

+        variance == other.variance
+            && type isEqualTo other.type
+      }
+      this == null && other == null -> true


I suppose this is already checked by this === other -> true?

Hmm, good point! I added the this === other check later 🙃 d3e6e36

udalov · 2020-01-15T15:53:56Z

kotlin/reflect/src/main/java/com/squareup/moshi/kotlin/reflect/KotlinJsonAdapter.kt

+    val annotations: List<Annotation>
+) {
+  val name get() = km.name
+  val isOptional get() = Flag.ValueParameter.DECLARES_DEFAULT_VALUE(km.flags)


Note that semantics of "is optional" are bit different from "declares default value". Not sure how important it is for your use case:

interface A { fun foo(x: String = "...") } interface B : A { override fun foo(x: String) }

x in B.foo is optional, although it does not declare default value.

KParameter.isOptional implements the "is optional" semantics.

Interesting, that's a good point. I think we can stick with just our default check since this is only ever checked on constructor parameters. Updated the naming of this though to reflect the default value semantic though! 164bd08

Co-Authored-By: Alexander Udalov <udalov@users.noreply.github.com>

Optimizes for happy path

ZacSweers · 2020-01-17T23:19:51Z

Updated with modernized proguard rules for this approach that also leverage R8's features where appropriate (based off of this PR for kotlin-reflect rules JetBrains/kotlin#2893)

@swankjesse want to get your sign-off before merging, especially with the platform types behavior change.

ZacSweers · 2020-01-17T23:34:25Z

kotlin/reflect/src/main/resources/META-INF/com.android.tools/proguard/moshi-kotlin.pro

+# Keep names of Kotlin classes but allow shrinking if unused
+-keepnames @kotlin.Metadata class *
+
+# Keep fields, constructors, and methods in Kotlin classes.
+-keepclassmembers @kotlin.Metadata class * {
+  <fields>;
+  <init>(...);
+  <methods>;
+}


@udalov @jsjeon I wonder if these should be added to the kotlin-reflect rules as well

For some reason I thought that the -keep class kotlin.Metadata rule would also keep all members. If that's not true, then your proposal seems reasonable.

This is slightly different, and rather it applies this rule only to classes annotated with Metadata, not the metadata annotation class itself

thinking on it more, I think it's probably best to omit these for now and have the guidance be that users apply keep rules as needed for their exact cases. Same with kotlin-reflect - the rules in place now make sure its machinery works, and users can just make sure they handle appropriate rules for classes that they plan to reflect on

Cleaned up in ad47a56

Oops, I should have paid attention to github alarms more often and carefully. Sorry for the late game.

@udalov

I thought that the -keep class kotlin.Metadata rule would also keep all members.

That's not true. The rule doesn't specify members, and thus all members are still eligible for shrinking and minification. Since members, such as k, d1, etc., are read all together (via metadata library), the correct rule is -keep class kotlin.Metadata { *; }.

But, as @ZacSweers already mentioned, rules here point to classes with metadata annotation, not the metadata definition itself.

Regarding having similar rules in kotlin-reflect, I would say no, because it's still up to users to add such rules if some classes (w/ or w/o metadata annotation) have reflective uses. Glad to see that you ended up with the same conclusion.

ZacSweers · 2020-02-27T20:21:22Z

Just a note for posterity - @swankjesse requested this not be merged until he can review as well

ZacSweers · 2020-07-31T01:55:15Z

Will reopen a new PR, I can't push to this branch anymore since I stepped back from the project 😬

ZacSweers · 2020-07-31T02:39:14Z

Continued here #1183

ZacSweers added 11 commits January 15, 2020 02:05

Add JvmDescriptorUtil.kt

2d290e1

Tweak some upcoming error message improvements

9f552da

Add kotlin-reflect for moshi-kotlin-tests

4fd2346

It's used there

Swap kotlinx-metadata-jvm for kotlin-reflect

b29af99

Kotlin-metadata-jvm rewrite of KotlinJsonAdapter.kt

3fae232

Formatting

529e853

Ignore platform types test for now pending CR

089470c

Include superclass properties

55fdaa0

Shade kotlinx-metadata

2a91781

Use a more specific replacement since code gen already has its own

d579b4c

Nix kotlin-reflect proguard rule

69da582

This was referenced Jan 15, 2020

Switch to kotlinx.reflect.lite when it’s sufficient #307

Open

Support subset needed by Moshi Kotlin/kotlinx.reflect.lite#14

Closed

Don't use API 26+ reflection APIs

91312fb

ZacSweers mentioned this pull request Jan 15, 2020

WIP benchmark Moshi-kotlin with metadata implementation ZacSweers/json-serialization-benchmarking#10

Closed

ZacSweers changed the title ~~Remove kotlin-reflect, replace with kotlinx-metadata-reflect~~ Remove kotlin-reflect, replace with kotlinx-metadata-jvm Jan 15, 2020

ZacSweers commented Jan 15, 2020

View reviewed changes

rharter approved these changes Jan 15, 2020

View reviewed changes

udalov reviewed Jan 15, 2020

View reviewed changes

kotlin/reflect/src/main/java/com/squareup/moshi/kotlin/reflect/KotlinJsonAdapter.kt Outdated Show resolved Hide resolved

udalov reviewed Jan 15, 2020

View reviewed changes

ZacSweers and others added 7 commits January 15, 2020 13:31

Use oneliners for descriptors

6cff563

Add IS_PRIVATE_TO_THIS

7e00f14

Co-Authored-By: Alexander Udalov <udalov@users.noreply.github.com>

Skip abbreviatedType check

36f945b

Nix redundant 'this == null && other == null' checks

d3e6e36

Rename to declaresDefaultValue for better semantics

164bd08

Check inherited fields too

a5a693b

Stick lazily cached field/method lookups in

da04f76

ZacSweers added 2 commits January 17, 2020 17:54

Separate declared vs non declared in lookups

cc079d6

Optimizes for happy path

Refresh proguard rules with optimized R8 handling

7a5579e

ZacSweers commented Jan 17, 2020

View reviewed changes

ZacSweers added Kotlin - reflect enhancement labels Jan 22, 2020

Nix blanket class keeps and mention in README instead

ad47a56

ZacSweers closed this Mar 28, 2020

ZacSweers reopened this Jul 29, 2020

ZacSweers closed this Jul 31, 2020

ZacSweers mentioned this pull request Jul 31, 2020

Remove kotlin-reflect, replace with kotlinx-metadata-jvm #1183

Closed

oldergod deleted the z/metadataReflect branch August 5, 2020 17:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove kotlin-reflect, replace with kotlinx-metadata-jvm #1076

Remove kotlin-reflect, replace with kotlinx-metadata-jvm #1076

ZacSweers commented Jan 15, 2020 •

edited

Loading

ZacSweers Jan 15, 2020

ZacSweers Jan 17, 2020

ZacSweers Jan 15, 2020

ZacSweers Jan 17, 2020

rharter left a comment

udalov Jan 15, 2020

ZacSweers Jan 15, 2020

udalov Jan 15, 2020

ZacSweers Jan 15, 2020

udalov Jan 15, 2020

ZacSweers Jan 15, 2020

udalov Jan 15, 2020

ZacSweers Jan 15, 2020

ZacSweers commented Jan 17, 2020

ZacSweers Jan 17, 2020

udalov Jan 22, 2020

ZacSweers Jan 22, 2020

ZacSweers Jan 25, 2020

ZacSweers Jan 26, 2020

jsjeon Feb 25, 2020

ZacSweers commented Feb 27, 2020

ZacSweers commented Jul 31, 2020

ZacSweers commented Jul 31, 2020

Remove kotlin-reflect, replace with kotlinx-metadata-jvm #1076

Remove kotlin-reflect, replace with kotlinx-metadata-jvm #1076

Conversation

ZacSweers commented Jan 15, 2020 • edited Loading

Stats

Two open questions

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

rharter 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

ZacSweers commented Jan 17, 2020

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

ZacSweers commented Feb 27, 2020

ZacSweers commented Jul 31, 2020

ZacSweers commented Jul 31, 2020

ZacSweers commented Jan 15, 2020 •

edited

Loading