Guava: ImmutableTable.get results in copyOf with actual copy being invoked

[victornoel]

I faced a strange performance situation with ImmutableTable.

Here is a stacktrace of what's happening (it's a profiling trace from async-profiler):

Basically, it seems that calling ImmutableTable.get() results in ImmutableMap.copyOf() being called, and even though there is a comment in rowMap() saying that copyOf is called only for casting (https://github.com/google/guava/blob/master/guava/src/com/google/common/collect/SparseImmutableTable.java#L97), in practice, there is an actual copy via toArray().

I believe this is not meant to happen, is it?

[victornoel]

For the record, the ImmutableTable was created using ImmutableTable.toImmutableTable

[samukce]

Hello, I was looking at this and I could understand why this is happening for get method. Basically, the classes that:

1. Inherits from AbstractTable;
2. It doesn't override the V get method;
3. It has in rowmap() implementation that use of copyOf or ImmutableMap.of

So based on those criteria I could find those classes that have this same behavior of creating news maps internally:

• SparseImmutableTable
• SingletonImmutableTable
• DenseImmutableTable

So, trying to summarize my founds: every use of rowMap()(+columnMap()) method in AbstractTable class it's a potential point of creating extra objects that it seems not necessary for such methods implementations such as: containsRow, containsColumn, containsValue, contains, get for ImmutableTable implementations;

My initial thought about it is that the rowMap and columnMap method it has been burdened with responsibilities:

1. to provide external data (which can be mutable or immutable data)
2. to provide support internally to access the data.

That said I can see this possible approach: To have an internal way to access the rowMap/columnMap in a protected class scope from ImmutableTable class so we could have those methods been implemented by this one, avoiding create unnecessary objects;

Something like

public abstract class ImmutableTable<R, C, V> extends AbstractTable<R, C, V> {
....
  protected abstract ImmutableMap<R, ImmutableMap<C, V>> immutableRowMap();

  @Override
  public boolean containsRow(@Nullable Object rowKey) {
    return Maps.safeContainsKey(immutableRowMap(), rowKey);
  }
...
}

final class DenseImmutableTable<R, C, V> extends RegularImmutableTable<R, C, V> {
  @Override
  protected ImmutableMap<R, ImmutableMap<C, V>> immutableRowMap() {
    return this.rowMap;
  }

Thoughts @kluever / @ronshapiro ?

[samukce]

I realized that the ImmutableMap.copyOf creation of new objects will not happen if: the map is it's an ImmutableMap and Is not SortedMap and is not PartialView; Also, if It is an EnumMap it will generate a copy and for the other map types will generate a copy too;

public static <K, V> ImmutableMap<K, V> copyOf(Map<? extends K, ? extends V> map) {
    if ((map instanceof ImmutableMap) && !(map instanceof SortedMap)) {
      @SuppressWarnings("unchecked") // safe since map is not writable
      ImmutableMap<K, V> kvMap = (ImmutableMap<K, V>) map;
      if (!kvMap.isPartialView()) {
        return kvMap;
      }
    } else if (map instanceof EnumMap) {
      @SuppressWarnings("unchecked") // safe since map is not writable
      ImmutableMap<K, V> kvMap = (ImmutableMap<K, V>) copyOfEnumMap((EnumMap<?, ?>) map);
      return kvMap;
    }
    return copyOf(map.entrySet());
  }

https://github.com/google/guava/blob/master/guava/src/com/google/common/collect/ImmutableMap.java#L426-L439

[samukce]

In the end, I could not reproduce my hypotheses either using the regular builder or the immutable collection (ImmutableTable.toImmutableTable). 😢 For both cases when we hit the copyOf method to rowMap() method through the get method, this map is never recreated returning the own object here Probably I'm missing something here... any help is welcome. 🤞

[cpovirk]

This does seem very strange, and it pops into my head from time to time.

The only even remotely plausible theory I have is that the ImmutableTable is an instance of the type from one classloader and the ImmutableMap call is using the type from another.

[edit: Actually, I don't think I buy this: The two references to ImmutableMap should refer to the same type, since they are both source references from `ImmutableTable. I think?]

[samukce]

I have tried to create a test to reproduce the issue but I could not. So created one just to guarantee no new object is created per call. Do you have any idea how to create this test for the cases you mentioned @cpovirk ?

https://github.com/google/guava/blob/51a76c04892de73578a2831897fb53fff9963e86/guava-tests/test/com/google/common/collect/ImmutableTableTest.java#L345-L363

[cpovirk]

For what it's worth, I'm confident that the code _would_ behave that way _if_ we were seeing that kind of interaction between multiple classloaders. What I'm not at _all_ confident in is that such an interaction was happening for the original poster.

If you're curious, it's definitely possible to test such things, but it's definitely a pain :) You can see some examples custom classloaders in our tests, such as in this test: https://github.com/google/guava/blob/081c486173032e6096c912e3c297c1d74eddcf93/guava-tests/test/com/google/common/util/concurrent/AbstractFutureInnocuousThreadTest.java#L48

[victornoel]

@cpovirk that's a good point... sorry for the late reply :)

I don't think it was in separate classloader when I experienced it, but I agree that this is really mystical if it wasn't the case!