Proposal: Replace foreach loop with Iterable.forEach in String.join(CharSequence, Iterable)

[Donald Raab]

Hi Claes,

> so the win comes from forEach on the synchronized collection implicitly
> being synchronized atomically, whereas foreach will synchronize on
> each iterator operation and thus allow racy modification of elements
> in between iteration, right?

Yes, this win will be applied to any type that does something special in forEach. For instance we have MultiReader collections in Eclipse Collections, which take a read lock on forEach, and throws UnsupportedOperationException on iterator. This change would make String.join safe to use for these as well, without requiring an explicit lock in client code.

> 
> Seems reasonable to consider, though it'd have to percolate through to most production deployments before you could use it - and even then
> it seems somewhat fragile. And isn't this just one of many utility
> methods that would have to be fixed to avoid issues with hidden
> iterators completely?
> 

Yes, this is one small step. Synchronized collections wouldn’t become less tricky to use, but there would be one less hidden iterator to worry about.

> String.join was recently changed to this by JDK-8265237[1]:
> 
>        for (CharSequence cs: elements) {
>            if (size >= elems.length) {
>                elems = Arrays.copyOf(elems, elems.length << 1);
>            }
>            elems[size++] = String.valueOf(cs);
>        }
> 

This looks like the code for the overloaded String.join which takes an array. I was referring to only the version that takes Iterable.

> Of course the body in the new code could be extracted to the same
> effect. The drawback in either implementation is that it would by
> necessity be a capturing lambda, so might add a small allocation
> overhead. I'm biased towards avoiding performance overheads, but maybe
> someone else can make a call on whether this is a worthwhile thing to
> attempt.
> 

There is a trade-off here. You remove the need to create an iterator for all types (due to foreach loop), and replace with forEach with a method reference, which IIRC might be slightly better than having a lambda. The underlying collection (let’s say ArrayList) will be called which will also not create an iterator but iterate directly over the ArrayList internal array.

Thanks,
Don

> Thanks!
> 
> /Claes
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8265237
> 
> On 2021-08-17 06:55, Donald Raab wrote:
>> The following code:
>> for (CharSequence cs: elements) {
>>     joiner.add(cs);
>> }
>> Can be replaced with:
>> elements.forEach(joiner::add);
>> This would have the minor benefit of making it safe to use String.join with synchronized collections without requiring a client side lock. There are likely other opportunities like this in the JDK.
>> Also, with the addition of forEach on Iterable, and Predicate in Java 8, the following design FAQ is outdated.
>> https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/doc-files/coll-designfaq.html#a6 <https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/doc-files/coll-designfaq.html#a6>
>> Thanks,
>> Don