filter/transform/asMap overloads for Sorted/Navigable types hinder Proguard

[cpovirk]

I recently knocked ~20Kb, 200 methods, and 20 classes off a Proguarded Android apps by replacing Maps.filterValues(map, predicate).clear() with Iterables.removeIf(map.values(), predicate). The app was using our JDK5 backport. If it had been using the mainline version, those savings would be 30Kb, 300 methods, and 30 classes.

But let's not pat ourselves on the back too much at these savings: A better way to look at it is that it was space Guava was wasting.

Maybe there's a way to enhance Proguard to detect that no Map passed to filterValues can ever be a SortedMap, at which it can prune the SortedMap and NavigableMap implementations that produced the bloat. But this is likely to be hard, and in fact it's likely to be impossible in some cases: If the Android system is returning a Map to you, there's no way for Proguard to know that neither this version of Android nor any future version will ever return a SortedMap.

For this reason, we should consider removing the Sorted/Navigable special case (basically, the instanceof checks) entirely. But... this will run afoul of the cardinal rule of overloading, which says here that calling filterKeys((Map) m) should do the same thing as filterKeys((SortedMap) m). Does that rule really matter? I'd say probably not, but I'd feel bad if I turned out to be wrong. We could always split the overloads out, java.util.Collections-style. But the naming here is uglier -- filterSortedKeys, etc. That's not too bad, I guess. (Originally we had the other methods in a separate NavigableMaps class... but we "fixed" that three years ago. But I don't think we'd want to go back to that, anyway.)

[cpovirk]

It occurs to me that a "similar" situation probably exists with MapMaker+CacheBuilder: The API returns an object that might have a bunch of extra features or might be very simple, but Proguard is going to have a hard time proving which it is. This could be particularly bad for the simple new MapMaker().makeMap() case, which just returns new ConcurrentHashMap() -- and note that Maps.newConcurrentMap() is implemented in terms of MapMaker!

[cpovirk]

And as soon as I say that, we get some data that makes Maps.newConcurrentMap() look not that bad -- a mere 31 methods. Of course, that's about 30 methods more than it really needs to use.

[cpovirk]

We should also look at the Proguard-friendliness of MultimapBuilder, particularly if we want for it to replace the old ways of creating a Multimap. Might it pull in all implementations even if you use only one?

[cpovirk]

It occurs to me that a simple test that calls MultimapBuilder.build() might not answer our question: Since Proguard uses whole-program optimization, we might need to measure specific MultimapBuilder invocation chains. But even that might not give One True Answer: If caller A requests hash keys and tree values and caller B requests tree keys and hash values, can Proguard figure out that it needs to support those two combinations but not hash keys plus hash values (which may require pulling in a separate HashMultimap implementation that wouldn't be used)?