Mailing-List: contact dev-help@harmony.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@harmony.apache.org
Received-SPF: neutral (athena.apache.org: 194.196.100.166 is neither permitted
 nor denied by domain of mark.hindess@googlemail.com)
Message-Id: <201008051413.o75EDcPZ010274@d06av01.portsmouth.uk.ibm.com>
In-reply-to: <201008041456.o74EuNhO004999@d12av03.megacenter.de.ibm.com>
References: <4C56E4F8.2040105@googlemail.com>
 <8652591182674059221@unknownmsgid>
 <AANLkTikT1+REkJr8H5nUK6E-UD-nHxh5WbwH2Gj3+MX5@mail.gmail.com>
 <201008041456.o74EuNhO004999@d12av03.megacenter.de.ibm.com>
Comments: In-reply-to Mark Hindess <mark.hindess@googlemail.com>
   message dated "Wed, 04 Aug 2010 15:56:23 +0100."
From: Mark Hindess <mark.hindess@googlemail.com>
To: dev@harmony.apache.org
Subject: Re: [classlib][luni] Collections classes - code reviews and
 optimisations
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Date: Thu, 05 Aug 2010 15:13:38 +0100


In message <201008041456.o74EuNhO004999@d12av03.megacenter.de.ibm.com>,
Mark Hindess writes:
> 
> I've now refactored the exception code to the start of the methods and
> shuffled some of the if else cases around a little.
> 
> Next I plan to look at:
> 
> 1) remove the following from addAll(...):
> 
>          if (this == collection) {
>              collection = (ArrayList)clone();
>          }
> 
>    since it is immediately followed by a call to collection.toArray() so
>    it should be unnecessary.

Done in r982377.  I also checked in the review comments we made to make it
easier to track them.

> 2) Change the fields to firstIndex and size.

I've now done this and checked it in at r982498.

I also made a minor change (below) to ensureCapacity that Oliver
observed does (accidentally) fix a bug that caused us to call a grow
method with 0 growSize and do a pointless array copy shuffling the
entire list.

     public void ensureCapacity(int minimumCapacity) {
-        if (array.length < minimumCapacity) {
+        int required = minimumCapacity - array.length;
+        if (required > 0) {
             // REVIEW: Why do we check the firstIndex first? Growing
             //         the end makes more sense
             if (firstIndex > 0) {
-                growAtFront(minimumCapacity - array.length);
+                growAtFront(required);
             } else {
-                growAtEnd(minimumCapacity - array.length);
+                growAtEnd(required);
             }
         }
     }

My initial (dacapo) benchmark runs on the original ArrayList, the
version with exception refactoring, the version without the spurious
clone and the version with the size changes seem to show that the
exception handling change was a bad idea but that we get back the lost
performance with the size change.  I'm just re-running the benchmarks
on a quiet(er) machine to confirm.  Will post details when they've
finished.

Regards,
 Mark.