RFR-8148748: ArrayList.subList().spliterator() is not late-binding

[Tagir F. Valeev]

Hello!

Sorry, I don't understand how passing AbstractList would help. We
still need direct access to the elementData array of the original
ArrayList for efficiency, and this field is also late-binding, so we
need to have a reference to the ArrayList as well even in SubList
case. SubList reference is also necessary, because we need to get size
and modCount from it. So we have need both ArrayList and SubList and
cannot replace them with single AbstractList. Note that my current
implementation does not introduce new virtual calls at all.

Surely it's possible to duplicate the Spliterator implementation. I
have some doubts that it's reasonable, because this would increase the
maintenance cost. Current Spliterator does the job pretty well for
both SubList and original ArrayList. Nevertheless, if you think it's
better I can create separate class (ArrayListSubListSpliterator?
ArrayListSubSpliterator? SubListSpliterator?) leaving the original
ArrayListSpliterator as is.

I'm just worrying a little that my changes might conflict with Ivan
Gerasimov's pending 8079136 issue, so probably it would be better to
wait till it's reviewed and pushed...

With best regards,
Tagir Valeev.

PS> Hi Tagir,

PS> It’s certainly easier to review if one avoids the temptation to
PS> change other things at the same time :-)

PS> There is a possible simplified approach to sharing one
PS> Spliterator impl between ArrayList and ArrayList.SubList. The
PS> initial (origin, fence) for ArrayList can be (0, -1) and
PS> ArrayList.SubList it can be (offset, -1), as before, then one can
PS> make it static passing in an instance of AbstractList:

PS>   if ((hi = fence) < 0) {
PS>     expectedModCount = lst.modCount; // shared field on AbstractList
PS>     hi = fence = index + lst.size(); // virtual call :-(
PS>   }

PS> Unfortunately that requires a virtual call to lst.size(). I don’t
PS> know if we could improve matters using a private interface
PS> contract to initialise the state e.g.:

PS>   if ((hi = fence) < 0) {
PS>     hi = initializer.init(this); // only two possible impls of initializer
PS>   }


PS> However, since ArrayList is quite a fundamental class my
PS> inclination is to provide a separate implementation, as SubList
PS> already does for ListIterator. Then we don’t perturb the optimal
PS> case, extra fields with branching or otherwise.

PS> Paul.

>> On 4 Feb 2016, at 16:55, Tagir F. Valeev <amaembo at gmail.com> wrote:
>> 
>> Hello!
>> 
>> Thank you for clarification. As far as I know currently JIT-compiled
>> null checks are almost always implicit (via trapped page-fault). Well,
>> ok, I'm not a Hotspot expert (probably for C1 this still matters). I
>> will keep this code as is.
>> 
>> Webrev was not changed:
>> http://cr.openjdk.java.net/~tvaleev/webrev/8148748/r1/
>> Any more reviews, please?
>> 
>> With best regards,
>> Tagir Valeev.
>> 
>> MB> On Wed, Feb 3, 2016 at 4:20 AM, Tagir F. Valeev <amaembo at gmail.com> wrote:
>> 
>>>> Some more thoughts about forEachRemaining:
>>>> 
>>>> To me it seems that
>>>> if ((a = lst.elementData) != null)
>>>> is a redundant check as well as in current ArrayList implementation
>>>> elementData is never null. So it can be replaced with simple
>>>> assignment.
>> 
>> MB> The null check for something that is provably non-null is a sign of
>> MB> the hand of Doug Lea.
>> MB> I'm often tempted to try to remove them from j.u.concurrent as well,
>> MB> but I resist.
>> MB> They may help hotspot generate better code.
>>