Reading SegmentInfo and FieldInfo Map and Set fields perform unecessary sorting and object creation

[blerer]

Description

The logic of DataInput.readMapOfStrings is the following:

  public Map<String, String> readMapOfStrings() throws IOException {
    int count = readVInt();
    if (count == 0) {
      return Collections.emptyMap();
    } else if (count == 1) {
      return Collections.singletonMap(readString(), readString());
    } else {
      Map<String, String> map = count > 10 ? new HashMap<>() : new TreeMap<>();
      for (int i = 0; i < count; i++) {
        final String key = readString();
        final String val = readString();
        map.put(key, val);
      }
      return Collections.unmodifiableMap(map);
    }
  }

If the number of element is greater than 10, the map that is used will be a TreeMap that will perfrom insert in O(log n) instead of an amortized O(1) and the returned map (HashMap or TreeMap) will be wrapped in an unmodifiable one.
The issue is that the sort and the wrapping appear to be in practice useless because SegmentInfo will perform a Map.copyOf on it (discarding the sort and the wrapping). FieldInfo do not perform any operation on top of the Map and expect an Unmodifiable HashMap (based on its code).
A similar thing happen with DataInput.readSetOfStrings

Based on those observations, it would make sense to remove the unecessary sorting and standardize the code on either Unmodifiable Maps or ImmutableMaps. Maps.copyOf is a noop on an ImmutableMap

Happy to provide a patch if the decision is to fix that logic. :-)

Version and environment details

No response

[msokolov]

I think you misread the code snippet -- when count > 10, a HashMap is used, not a TreeMap. I'm not sure what the reason for using a TreeMap for smaller maps is, maybe just some convenience?

Tricky ternary operator! I thought we had banned those, hmm

[blerer]

You are right. I thought that it was for some debugging reason when the Map started to be large enough and did not read the code correctly.

[dweiss]

I don't see the point of using a treemap there at all, to be honest. I wonder what the reason for doing that was but I don't think it'll win over a plain HashMap, ever.

[blerer]

@dweiss, @msokolov I have created a small PR to address this problem. Tell me if it makes sense to you.