[jira] [Comment Edited] (GEODE-10112) Add ConcurrentHashMap support to DataSerializer

["Kirk Lund (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/GEODE-10112?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17503080#comment-17503080 ] 

Kirk Lund edited comment on GEODE-10112 at 3/8/22, 5:44 PM:
------------------------------------------------------------

Some initial feedback from [~jbarrett]:

We should not be serializing specific types of Map at all. Type specific serialization causes issues when trying to deserialize on non-Java systems and make migrations to different, maybe more performant, Map implementations more complicated.

We should have a writeMap(DataOutput out, Map<?. ?> map) and <K, V> Map<K, V> readMap(DatatInput in) and maybe a <K, V> Map<K, V> readMap(DatatInput in, Map<K, V> map) that will utilize the pre-constructed map for deserialization directly. Additionally I would argue for <M extends Map<K, V>, K, V> M readMap(DataInput in, MapConstructor mapConstructor) for optimizing the construction for the size of the map.
{noformat}
  @FunctionalInterface
  public interface MapConstructor<M extends Map<K, V>, K, V> {
    M construct(int size);
  }

  public static <M extends Map<K, V>, K, V> M readMap(DataInput in, MapConstructor<M, K, V> mapConstructor)
      throws IOException, ClassNotFoundException {
    InternalDataSerializer.checkIn(in);

    int size = InternalDataSerializer.readArrayLength(in);
    if (size == -1) {
      return null;
    } else {
      M map = mapConstructor.construct(size);
      for (int i = 0; i < size; i++) {
        K key = DataSerializer.readObject(in);
        V value = DataSerializer.readObject(in);
        map.put(key, value);
      }

      if (logger.isTraceEnabled(LogMarker.SERIALIZER_VERBOSE)) {
        logger.trace(LogMarker.SERIALIZER_VERBOSE, "Read Map with {} elements: {}", size, map);
      }

      return map;
    }
  }
{noformat}
The legacy readHashMap:
{noformat}
  public static <K, V> HashMap<K, V> readHashMap(DataInput in)
      throws IOException, ClassNotFoundException {
    return readMap(in, HashMap::new);
  }
{noformat}
And to read a ConcurrentHashMap:
{noformat}
  public void someMethod(DataInput in)
      throws IOException, ClassNotFoundException {
    ConcurrentHashMap<String, String> map = readMap(in, ConcurrentHashMap::new);
    ...
  }
{noformat}
Because someday maybe we need to do this change the type to something faster:
{noformat}
  public void someMethod(DataInput in)
      throws IOException, ClassNotFoundException {
    SuperFastConcurrentHashMap<String, String> map = readMap(in, SuperFastConcurrentHashMap::new);
    ...
  }
{noformat}
Without this generic Map capability we are stuck with:
{noformat}
  public void someMethod(DataInput in)
      throws IOException, ClassNotFoundException {
    SuperFastConcurrentHashMap<String, String> map = new SuperFastConcurrentHashMap(readConcurrentHashMap(in);
    ...
  }
{noformat}
Which has the unfortunately side affect of deserializing to a types we can’t use then populating the type that we want.


was (Author: klund):
Some initial feedback from [~jbarrett]:

We should not be serializing specific types of Map at all. Type specific serialization causes issues when trying to deserialize on non-Java systems and make migrations to different, maybe more performant, Map implementations more complicated.

We should have a writeMap(DataOutput out, Map<?. ?> map) and <K, V> Map<K, V> readMap(DatatInput in) and maybe a <K, V> Map<K, V> readMap(DatatInput in, Map<K, V> map) that will utilize the pre-constructed map for deserialization directly. Additionally I would argue for <M extends Map<K, V>, K, V> M readMap(DataInput in, MapConstructor mapConstructor) for optimizing the construction for the size of the map.
{{{
  @FunctionalInterface
  public interface MapConstructor<M extends Map<K, V>, K, V> {
    M construct(int size);
  }

  public static <M extends Map<K, V>, K, V> M readMap(DataInput in, MapConstructor<M, K, V> mapConstructor)
      throws IOException, ClassNotFoundException {
    InternalDataSerializer.checkIn(in);

    int size = InternalDataSerializer.readArrayLength(in);
    if (size == -1) {
      return null;
    } else {
      M map = mapConstructor.construct(size);
      for (int i = 0; i < size; i++) {
        K key = DataSerializer.readObject(in);
        V value = DataSerializer.readObject(in);
        map.put(key, value);
      }

      if (logger.isTraceEnabled(LogMarker.SERIALIZER_VERBOSE)) {
        logger.trace(LogMarker.SERIALIZER_VERBOSE, "Read Map with {} elements: {}", size, map);
      }

      return map;
    }
  }
}}}
The legacy readHashMap:
{{{
  public static <K, V> HashMap<K, V> readHashMap(DataInput in)
      throws IOException, ClassNotFoundException {
    return readMap(in, HashMap::new);
  }
}}}
And to read a ConcurrentHashMap:
{{{
  public void someMethod(DataInput in)
      throws IOException, ClassNotFoundException {
    ConcurrentHashMap<String, String> map = readMap(in, ConcurrentHashMap::new);
    ...
  }
}}}
Because someday maybe we need to do this change the type to something faster:
{{{
  public void someMethod(DataInput in)
      throws IOException, ClassNotFoundException {
    SuperFastConcurrentHashMap<String, String> map = readMap(in, SuperFastConcurrentHashMap::new);
    ...
  }
}}}
Without this generic Map capability we are stuck with:
{{{
  public void someMethod(DataInput in)
      throws IOException, ClassNotFoundException {
    SuperFastConcurrentHashMap<String, String> map = new SuperFastConcurrentHashMap(readConcurrentHashMap(in);
    ...
  }
}}}
Which has the unfortunately side affect of deserializing to a types we can’t use then populating the type that we want.

> Add ConcurrentHashMap support to DataSerializer
> -----------------------------------------------
>
>                 Key: GEODE-10112
>                 URL: https://issues.apache.org/jira/browse/GEODE-10112
>             Project: Geode
>          Issue Type: Improvement
>          Components: wan
>            Reporter: Kirk Lund
>            Assignee: Kirk Lund
>            Priority: Major
>              Labels: GeodeOperationAPI
>
> Add ConcurrentHashMap support to DataSerializer. This will improve performance of Gateway messages that have fields related to regionToDispatchedKeysMap which contain values of type ConcurrentHashMap. Messages include ParallelQueueRemovalMessage.



--
This message was sent by Atlassian Jira
(v8.20.1#820001)