why not use a map for class->adapter mapping

[pouloghost]

Issue Summary

why not use a map for class->adapter mapping. currently is using a complicated package-> index-> subFactory-> if-else logic, which is neither efficient nor easy to understand.

Expected Behavior

just generate a map<Class, TypeAdapter> which will be extremely easy to debug and read

[anthonycr]

There are a couple problems with using a map rather than the current implementation

• One problem with creating something like a static HashMap is that we need to inject the Gson dependency into type adapters. Not all type adapters need the Gson dependency, but certain cases need it and therefore we cannot guarantee that all use cases will fit into a static HashMap. Usage of the ObjectTypeAdapter requires injecting the Gson instance.
• There are also use cases for which we must have access to the TypeToken instance at runtime to infer certain information about the type, which would preclude us from adding those cases to the map.

For the other cases, I will look into using HashMap as that would be easier to grok and could be more performant since it only has to do an O(1) jump + 1 reference comparison vs N reference comparisons. I'll make a benchmark and see if there are any performance impacts. If the performance is equal or better, I think we can switch to using HashMap for the readability gain.

[anthonycr]

I benchmarked the if/else vs using a HashMap and sure enough, it's 3-4x faster. I'll make the change to switch to a static map for stateless TypeAdapters.

There was a bug in my original benchmark that resulted in unnecessary object creation. I have updated it to remove the unnecessary object creation, see below. The benchmark does not show a significant performance difference between the two implementations, and if anything shows that using the HashMap to be slightly slower than using a bunch of if blocks. Given this information, I don't think that this change will result in any performance gains.

Let me know if you find a problem in the benchmark or have other data that shows that using the map is faster.

Updated `HashMap` vs `if/else` benchmark

class Scratch {

private val classes = listOf(
        AlternateNameModel::class.java,
        AlternateNameModel1::class.java,
        BaseExternalModel::class.java,
        BooleanFields::class.java,
        EnumWithFieldsModel::class.java,
        ExternalAbstractClass::class.java,
        ExternalModel1::class.java,
        ExternalModel2::class.java,
        ExternalModelGeneric::class.java,
        ExternalModelGeneric1::class.java,
        ModelWithNestedInterface::class.java,
        NativeJavaModel::class.java,
        NativeJavaModelExtension::class.java,
        NativeJavaModelExtensionWithoutAnnotation::class.java,
        RawGenericField::class.java,
        NullFields::class.java,
        PrivateMembers::class.java,
        WildcardModel::class.java,
        DynamicallyTypedModel::class.java,
        DynamicallyTypedWildcard::class.java,
        AbstractDataList::class.java,
        SuperAbstractDataList::class.java,
        ConcreteDataList::class.java
)

private val classMap = hashMapOf(
        AlternateNameModel::class.java to "AlternateNameModel",
        AlternateNameModel1::class.java to "AlternateNameModel1",
        BaseExternalModel::class.java to "BaseExternalModel",
        BooleanFields::class.java to "BooleanFields",
        EnumWithFieldsModel::class.java to "EnumWithFieldsModel",
        ExternalAbstractClass::class.java to "ExternalAbstractClass",
        ExternalModel1::class.java to "ExternalModel1",
        ExternalModel2::class.java to "ExternalModel2",
        ExternalModelGeneric::class.java to "ExternalModelGeneric",
        ExternalModelGeneric1::class.java to "ExternalModelGeneric1",
        ModelWithNestedInterface::class.java to "ModelWithNestedInterface",
        NativeJavaModel::class.java to "NativeJavaModel",
        NativeJavaModelExtension::class.java to "NativeJavaModelExtension",
        NativeJavaModelExtensionWithoutAnnotation::class.java to "NativeJavaModelExtensionWithoutAnnotation",
        RawGenericField::class.java to "RawGenericField",
        NullFields::class.java to "NullFields",
        PrivateMembers::class.java to "PrivateMembers",
        WildcardModel::class.java to "WildcardModel",
        DynamicallyTypedModel::class.java to "DynamicallyTypedModel",
        DynamicallyTypedWildcard::class.java to "DynamicallyTypedWildcard",
        AbstractDataList::class.java to "AbstractDataList",
        SuperAbstractDataList::class.java to "SuperAbstractDataList",
        ConcreteDataList::class.java to "ConcreteDataList"
)

private fun <T : Any> switchName(clazz: Class<T>): String? {
    if (clazz == AlternateNameModel::class.java) {
        return "AlternateNameModel"
    }
    if (clazz == AlternateNameModel1::class.java) {
        return "AlternateNameModel1"
    }
    if (clazz == BaseExternalModel::class.java) {
        return "BaseExternalModel"
    }
    if (clazz == BooleanFields::class.java) {
        return "BooleanFields"
    }
    if (clazz == EnumWithFieldsModel::class.java) {
        return "EnumWithFieldsModel"
    }
    if (clazz == ExternalAbstractClass::class.java) {
        return "ExternalAbstractClass"
    }
    if (clazz == ExternalModel1::class.java) {
        return "ExternalModel1"
    }
    if (clazz == ExternalModel2::class.java) {
        return "ExternalModel2"
    }
    if (clazz == ExternalModelGeneric::class.java) {
        return "ExternalModelGeneric"
    }
    if (clazz == ExternalModelGeneric1::class.java) {
        return "ExternalModelGeneric1"
    }
    if (clazz == ModelWithNestedInterface::class.java) {
        return "ModelWithNestedInterface"
    }
    if (clazz == NativeJavaModel::class.java) {
        return "NativeJavaModel"
    }
    if (clazz == NativeJavaModelExtension::class.java) {
        return "NativeJavaModelExtension"
    }
    if (clazz == NativeJavaModelExtensionWithoutAnnotation::class.java) {
        return "NativeJavaModelExtensionWithoutAnnotation"
    }
    if (clazz == RawGenericField::class.java) {
        return "RawGenericField"
    }
    if (clazz == NullFields::class.java) {
        return "NullFields"
    }
    if (clazz == PrivateMembers::class.java) {
        return "PrivateMembers"
    }
    if (clazz == WildcardModel::class.java) {
        return "WildcardModel"
    }
    if (clazz == DynamicallyTypedModel::class.java) {
        return "DynamicallyTypedModel"
    }
    if (clazz == DynamicallyTypedWildcard::class.java) {
        return "DynamicallyTypedWildcard"
    }
    if (clazz == AbstractDataList::class.java) {
        return "AbstractDataList"
    }
    if (clazz == SuperAbstractDataList::class.java) {
        return "SuperAbstractDataList"
    }
    if (clazz == ConcreteDataList::class.java) {
        return "ConcreteDataList"
    }
    return null
}

private fun <T : Any> mapName(clazz: Class<T>): String? {
    return classMap[clazz]
}

@Test
fun scratch() {
    benchmark("RESET") { }

    benchmark("MAP") { _ ->
        classes.forEach {
            requireNotNull(mapName(it))
        }
    }

    benchmark("SWITCH") { _ ->
        classes.forEach {
            requireNotNull(switchName(it))
        }
    }
}

private inline fun benchmark(name: String, block: (Int) -> Unit) {
    val start = System.nanoTime()
    repeat(1000000, block)
    val end = System.nanoTime()
    println("BENCHMARK $name FINISHED IN ${TimeUnit.NANOSECONDS.toMillis(end - start)} MILLISECONDS")
}

}

Given that there are no performance benefits to switching to a map, the main differences that I see are:

• for map usage
  • less code
  • easier to read
  • single point of return for stateless type adapters (easier to use debugger on)
• for if/else usage
  • reduced memory overhead, no duplication of the Map that's internally used by Gson
  • easier to debug individual type adapter creation
  • if/else isn't all that difficult to understand, it's still linear

There is some difficulty in adding in an abstraction to determine whether a type adapter is stateless and can be statically held in a map, so I'm going to hold off trying to implement a static map until it's clear that the benefits outweigh using the if/else.

[yasirmhd]

@anthonycr given that this is used by Android apps, I believe there was a cost involved in loading of classes which would be incurred during a cold boot scenario. We should check for those before we make this change.

cc : @anirudhramanan who would have more info on the benchmarks we ran for this.

[anirudhramanan]

As @yasirmhd mentioned, the cold boot time increased significantly when we tried the map implementation for the same. (shooted up to 4.2 sec from 2.4 seconds)

Reasons being:

1. With the map implementation, all the classes which were being mapped to its type adapter gets loaded into memory which increased the cold boot time, as well as the memory. Also, since most of the type-adapters have TYPE_TOKEN as static fields, these also gets loaded into memory (and since TypeToken.get() is a heavy operation), it had a significant impact on boot time.

2. We made another optimisation where we had to split the main Stag.Factory into separate sub factories which are being generated at a package level (This was done because with increase in number of models, the file size of Stag.Factory used to increase, and hence it took more time to load this single file). Since we are generating TypeAdapterFactory at a package level, we map a given package to a TypeAdapterFactory which gets lazily initialised. This also makes sure the size of the map/list does not increase with the addition of new classes in the existing package.

These optimisations helped us bring down the cold boot time significantly.