Sugmanue/rpcv2 improve cbor performance 04

[sugmanue]

Motivation and Context

Improvement to the lookup performance of marshallers and traits.

Both traits and marshalling types are open classes but with well known, finite, and, statically defined instances. We can improve the performance of the lookups of these values by enumerating them using an enum and associating the enum values to the concrete types, and then using a two level lookup: the fast level 1 uses the an enum map and the associate enumeration value as key, while the l2 uses a regular map with the values as keys as they are. This change improves not only RPCv2, but all the other protocols as well.

This change aims to reduce, or remove the hump highlighted in the flamegraph below

Before

Benchmark                              (protocol)  (size)  Mode  Cnt      Score     Error  Units
JsonMarshallerBenchmark.unmarshall  smithy-rpc-v2   small  avgt    5   3415.127 ±  64.585  ns/op
JsonMarshallerBenchmark.unmarshall  smithy-rpc-v2  medium  avgt    5   5668.994 ±  16.272  ns/op
JsonMarshallerBenchmark.unmarshall  smithy-rpc-v2     big  avgt    5  19475.973 ± 133.802  ns/op
JsonMarshallerBenchmark.unmarshall       aws-json   small  avgt    5  10150.744 ±  28.942  ns/op
JsonMarshallerBenchmark.unmarshall       aws-json  medium  avgt    5  21037.573 ± 139.545  ns/op
JsonMarshallerBenchmark.unmarshall       aws-json     big  avgt    5  86466.945 ± 709.876  ns/op

And zoomed in flamegraph

After

Benchmark                              (protocol)  (size)  Mode  Cnt      Score      Error  Units
JsonMarshallerBenchmark.unmarshall  smithy-rpc-v2   small  avgt    5   3166.243 ±  151.799  ns/op
JsonMarshallerBenchmark.unmarshall  smithy-rpc-v2  medium  avgt    5   5349.115 ±  172.858  ns/op
JsonMarshallerBenchmark.unmarshall  smithy-rpc-v2     big  avgt    5  17517.889 ±   41.898  ns/op
JsonMarshallerBenchmark.unmarshall       aws-json   small  avgt    5  10178.752 ±  167.762  ns/op
JsonMarshallerBenchmark.unmarshall       aws-json  medium  avgt    5  20440.322 ±  223.031  ns/op
JsonMarshallerBenchmark.unmarshall       aws-json     big  avgt    5  83252.862 ± 1159.517  ns/op

And zoomed in flamegraph

Full flamegraph after this change

Modifications

Testing

Screenshots (if appropriate)

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)

Checklist

• I have read the CONTRIBUTING document
• Local run of mvn install succeeds
• My code follows the code style of this project
• My change requires a change to the Javadoc documentation
• I have updated the Javadoc documentation accordingly
• I have added tests to cover my changes
• All new and existing tests passed
• I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
• My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

• I confirm that this pull request can be released under the Apache 2 license

[sonarqubecloud]

Quality Gate failed

Failed conditions
49.3% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[joviegas]

It will be nice if we rename it with something more meaningful like createTraitsWithType and createTraitsWithNoType or something which tells the difference

[sugmanue]

I feel like level 1 and level 2 (similar wording as used to describe caches, CPU and others) is more meaningful. Type vs NoType does not say much about why those are different.

[joviegas]

Optional : We can concise this as below ,

private static Map<Class<? extends Trait>, Trait> createL2Traits(Map<Class<? extends Trait>, Trait> traits) {
    return traits.entrySet().stream()
        .filter(entry -> entry.getValue().type() == null) 
        .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
}

[sugmanue]

I'm purposefully avoiding streams, in the same way that I removed some code using them before, those tend to create garbage and pressure on the garbage collection which is reflected in the benchmarks. For critical code paths I think is better to just use plain old for loops.

[joviegas]

import java.math.BigDecimal;
import java.time.Instant;
import java.util.*;

public enum MarshallingKnownType {
    NULL,
    STRING,
    INTEGER,
    LONG,
    FLOAT,
    DOUBLE,
    BIG_DECIMAL,
    BOOLEAN,
    INSTANT,
    SDK_BYTES,
    SDK_POJO,
    LIST,
    MAP,
    SHORT,
    BYTE,
    DOCUMENT;


    private static final Map<Class<?>, MarshallingKnownType> TYPE_MAP = new HashMap<>();

    static {
        // Populate the map with class-to-known-type mappings
        TYPE_MAP.put(Void.class, NULL);
        TYPE_MAP.put(String.class, STRING);
        TYPE_MAP.put(Integer.class, INTEGER);
        TYPE_MAP.put(Long.class, LONG);
        TYPE_MAP.put(Float.class, FLOAT);
        TYPE_MAP.put(Double.class, DOUBLE);
        TYPE_MAP.put(BigDecimal.class, BIG_DECIMAL);
        TYPE_MAP.put(Boolean.class, BOOLEAN);
        TYPE_MAP.put(Instant.class, INSTANT);
        TYPE_MAP.put(SdkBytes.class, SDK_BYTES);
        TYPE_MAP.put(SdkPojo.class, SDK_POJO);
        TYPE_MAP.put(List.class, LIST);
        TYPE_MAP.put(Map.class, MAP);
        TYPE_MAP.put(Short.class, SHORT);
        TYPE_MAP.put(Byte.class, BYTE);
        TYPE_MAP.put(Document.class, DOCUMENT);
    }


    public static MarshallingKnownType from(Class<?> clazz) {
        return TYPE_MAP.getOrDefault(clazz, null); // Returns null if class is not found
    }
}

We can avoid the if-else checks by using Hashmap for constant time look up, same for the other Enum

WDYT ?

[sugmanue]

I don't see much difference, using a map would consume memory which will be used only once (when the SdkPojo class is loaded and not longer after that), in that sense using an multi-branch if feels better. What do you think?

[joviegas]

The one I suggested also enhances the readability , thus recommended it.

[joviegas]

When will we enternull case ?
Why aren't we throwing Exception in this case ?

[sugmanue]

Unfortunately throwing is not an option, this was released as open ended class, throwing would break backwards compatibility, and, there are uses of this class outside of the known types. For instance, this one.

[joviegas]

I might be missing something , while doing a get we do below

        public Object get(MarshallingType<?> marshallingType) {
            MarshallingKnownType knownType = marshallingType.getKnownType();
            if (knownType != null) {
                return l1registry.get(knownType);
            }
            return l2registry.get(marshallingType);
        }

since predefined (line 38 to 68) MarshallingType are also predefined in MarshallingKnownType , what cases we will end up in predefined type not present in l1registry but present in l2registry ?
I was thinking if we can remove (line 38 to 68) is redendant

[sugmanue]

That would be nice, unfortunately this class is already released as open, so customers can define their own which would go into the L2 registry. See also this other example in our code base.