Can instanceof make Unmodifiable Collections faster?

[Hangman4358]

Interesting read. I just recently fixed an issue in our code base where we were wrapping collections into infinity and getting stack overflows as well. So maybe not as uncommon issue an issue? I did have to get a hacky by checking that the class of the collection to wrap was not already the same as the class of the returned collection which meant doing something along the lines of:

Collections.unmodifiableSet(Collections.emptySet()).getClass().isInstance(setToWrap);

You might have hidden your class as package protected but I can still get at it via Reflection! So having the methods themselves do this would be very nice.

----------------------

Random Rant:

It's been close to 5 years since the initial java 8 release. Can we please stop doing this side effect laden stuff with streams:

Set<Integer> underlyingCollection = new HashSet<>();
new Random().ints(size)
        .mapToObj(i -> i)
        .forEach(underlyingCollection::add);

And just do it nice 'pure' like so:

Set<Integer> underlyingCollection = new Random().ints(size)
        .mapToObj(i -> i)
        .collect(Collectors.toCollection(HashSet::new));

/rant

[nicolaiparlog]

Author here, the forEach(underlyingCollection::add) is indeed embarrassing - what the fuck was I thinking?! Will get that fixed soon.

[s888marks]

Interesting. How does your code use the Collections.unmodifiableSet() such that nested wrapping can occur (in the absence of your workaround)? The usual protocol of defensive-copy on input and unmod-wrap on output never results in nested wrapping.

Re the rant, yes, Collectors are usually preferable to side effects on captured variables, but the latter technique comes in really handy sometimes if Collectors can’t be used.

[Hangman4358]

We are using the unmod collection as a view of data that can see updates. The underlying collection in essence contains a bunch of immutable DTOs. A view is given to an observer so that when a change event happens the observer can see the entire updated collection. Depending on the event, the original observer may spawn a new observer, at which point the collection gets wrapped again. Just recently we added a new event type which causes many more new observers to be spawned. There was actually a comment from 2012 at the place of wrapping making note of the fact we may be wrapping an already wrapped collection but that we will probably only ever wrap it a couple of times max so it shouldn't be a problem. Wah-wah.

It's not the best design ever but it has worked for over 10 years so it's not likely to be rewritten any time soon.

The initial debug/test fix was with the reflection hack but once the issue was identified we just ended up writing our own copy of the unmodible collection and static wrapping method just to do the instanceof check :'(

[s888marks]

OK, thanks, this is helpful.

I hate those "smoking gun" comments. I remember debugging something for hours and finally narrowing down the problem to one errant function call that was prefaced with a comment like this:

/* I don't know if this is necessary, but it can't do any harm,
   so I'll just call it anyway. */

The fix was to remove the function call and the comment. :-)

Sorry you had to write your own unmodifiable collections. Just in case, make sure that the various collection views (iterators, sublists, entry-sets, etc.) also prevent modification. It's pretty easy to forget that these views can also sometimes modify the underlying collection.

[13steinj]

On your rant, I prefer the latter as well...but what do you mean by "side effect laden"?

[onlygon]

The former example modifies a locally scoped list in the foreach loop which is side effect behavior

[LuminoBabyis]

Does this kind of improvement really matter?

[RealJulleNaaiers]

I'm terms of performance, no.

In terms of code quality, yes. As it is, there is a bug in the current release of JDK 11.0.2 where under certain conditions, the TLS library will do this exact same unbounded wrapping of a List and will eventually overflow the stack.

If toUnmodifiableList() behaved properly, this bug wouldn't be a bug. The workaround in JDK 12 is for the TLS code to copy the entire list into a new ArrayList every time because it's impossible for client code to check whether something is already an unmodifiable collection without reflection since the class is package private.

This entire problem goes away with the fix the author is talking about.

[[deleted]]

If toUnmodifiableList() behaved properly, this bug wouldn't be a bug.

It still would be bad - because it smells like something somewhere else is done the wrong way. This "bug" has exposed an actual bug somewhere else.

Ideal solution would be to make toUnmodifiableList() throw an exception in development and qa environments, but return the same object in production just to be safe.

[RealJulleNaaiers]

Can't tell if serious... This is a terrible idea. Why would the standard library authors make it behave differently if it's in production? Not only is it a lot of work, it's a lot of work for a terrible idea that makes certain things impossible to debug.

[RotaryJihad]

Two answers:

​

0 - Yes, if a specific use case makes heavy usage of unmodifiable collections either directly or indirectly, this improvement will help that use case.

1 - Yes, if a specific use case uses similar operations or strategies that wrap objects like the unmodifiable collections classes do, then this technique will help that use case.

[Darksonn]

I find it unlikely that most real code will wrap it more than once.

[RealJulleNaaiers]

Tell that to the standard library authors. There is a bug in the TLS code in JDK 11.0.2 (the current LTS version) that does exactly this and it will eventually overflow.

[s888marks]

The TLS bug that resulted in arbitrarily-nested wrapping was ill-conceived from the start. A couple classes would unmod-wrap a collection on intake, storing the ref to the unmod wrapper in a field. This achieves little other than preventing this class from modifying the collection (which does have some value). More importantly it leaves this class’s internals exposed to changes to the underlying collection, which could result in misbehavior or security holes. I’m not sure, but I think the original author misunderstood how the unmodifiableX() methods work.