Mailing-List: contact dev-help@harmony.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@harmony.apache.org
Received-SPF: neutral (nike.apache.org: 195.212.29.141 is neither permitted
 nor denied by domain of mark.hindess@googlemail.com)
Message-Id: <200804221702.m3MH2OZV017051@d06av01.portsmouth.uk.ibm.com>
In-reply-to: <4bebff790804220423l559b29b9ye2762a45b3369604@mail.gmail.com> 
References: <4bebff790804181153w61231384y436c4c4aa9500ba4@mail.gmail.com>
 <728dc7fa0804181201n9debd8i4ea43f1e95bf236f@mail.gmail.com>
 <480C8D85.8040705@gmail.com>
 <4bebff790804210552k13ed10a1u1a0daf4d43e33c43@mail.gmail.com>
 <480D9D3D.6080408@gmail.com>
 <4bebff790804220423l559b29b9ye2762a45b3369604@mail.gmail.com>
Comments: In-reply-to "Aleksey Shipilev" <aleksey.shipilev@gmail.com>
   message dated "Tue, 22 Apr 2008 15:23:40 +0400."
From: Mark Hindess <mark.hindess@googlemail.com>
To: dev@harmony.apache.org
Subject: Re: [classlib][luni][performance] IdentityHashMap implementation 
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Date: Tue, 22 Apr 2008 18:02:19 +0100


On 22 April 2008 at 15:23, "Aleksey Shipilev" <aleksey.shipilev@gmail.com> 
wrote:
> Tim,
> 
> I support your proposal and try to make distinction between
> proof-of-concept and clean patches.
> 
> Performance improvements could be notorious work, requiring a lot of
> resources. The bad thing is, they could give nothing after the work is
> done, that's why we are doing thought experiments and proof-of-concept
> patches before actually implementing something. Proof-of-concept may
> also give a hint on what maximum improvement we can get and set the
> expectations for clean patch.
> 
> So we are not looking for hiding the problems, rather we're exploring
> what could it worth.
> 
> F.ex. the experiments on IdentityHashMap performance finally led to
> clean, non-breaking, consistent implementation, ready to be committed
> patch [1]. Can we focus on reviewing it?

"clean [snip] patch"?  Okay, I'll take the bait...

1) It is good to fix the assert in the test case, but if you are going to
change it at all you might as well replace:

  assertTrue("TestB. removed entries incorrectly", map.size() == 11 && 
!result);

with:

  assertEquals("TestB. removed entries incorrectly, size is not correct",
               11, map.size());
  assertFalse("TestB. removed entries incorrectly, result is not correct",
              result);


2) I think the IdentityHashMap javadoc comment change removes too much
important information.


3) I don't understand why you make arbitrary whitespace changes - such
as rewriting:

public class IdentityHashMap<K, V> extends AbstractMap<K, V> implements
        Map<K, V>, Serializable, Cloneable {

to be longer than 80-characters.


4) You rename DEFAULT_MAX_SIZE to DEFAULT_SIZE but I don't really see
the relevance of this to the changes.


5) You seem to remove a number of correct comments from the
declarations of threshold, DEFAULT_MAX_SIZE, loadFactor, and modCount.

I almost I gave up reading the patch at this point out of frustration
at the number of irrelevant changes.  Such as:

a) the re-ordering of often unchanged public methods - previously the
public methods were grouped by similar functionality which seems logical
to me but now they are in alphabetical order.  This makes the patch much
bigger and harder to read.  If there is a good reason to re-order the
methods, do it in a separate patch.


b) The arbitrary changes to javadoc like:

     /**
-     * Searches this Map for the specified value.
-     * 
-     * 
+     * Searches this IdentityHashMap for the specified value.
+     *
      * @param value
      *            the object to search for
-     * @return true if <code>value</code> is a value of this Map, false
+     * @return true if <code>value</code> is a value of this IdentityHashMap, 
false
      *         otherwise
      */

c) The arbitrary renaming of IdentityHashMapEntry to Entry.

Is it unrealistic to expect clean patches with justifications for
changes?  Can we optimise patches as well as code?

If I apply the patch and then revert the whitespace changes,
javadoc reformatting, renaming of DEFAULT_MAX_SIZE, renaming of
IdentityHashMapEntry, etc. the the resulting svn diff contains at least
33% fewer +/- lines than the one on the JIRA and is still functionally
the same.

As someone who tries to read svn commit messages (and all committers or
would-be-committers should be doing this to some extent!) patches like
this are a nightmare.

Regards,
-Mark.

> 
> Thanks,
> Aleksey.
> 
> [1] https://issues.apache.org/jira/browse/HARMONY-5771
> 
> On Tue, Apr 22, 2008 at 12:09 PM, Tim Ellison <t.p.ellison@gmail.com> wrote:
> > Aleksey Shipilev wrote:
> >
> > > Right, Tim. It was just the proof-of-concept "Does IdentityHashMap
> > > tuning worth it?".
> > >
> >
> >  I think it is unhelpful to mix actual, concrete suggestions for performanc
> e
> > improvements, with proof-of-concept ideas that break the specification or
> > security.
> >
> >  Please can people mark clearly in their mail whether they are suggesting
> > the idea only as "a thought experiment", otherwise somebody may apply the
> > change without properly understanding the consequences.
> >
> >  I realize the onus is on the committer to ensure it is safe but hiding suc
> h
> > problems is, as I said, unhelpful.
> >
> >  Thanks,
> >  Tim
> >
> >
> >
> >
> >
> > > On Mon, Apr 21, 2008 at 4:50 PM, Tim Ellison <t.p.ellison@gmail.com>
> > wrote:
> > >
> > > > Sergey Salishev wrote:
> > > >
> > > >
> > > > > It also should be noticed that replacing the IdentityHashMap with
> > HashMap
> > > > >
> > > > in
> > > >
> > > > > Thread impl gives 4.5x boost on ThreadLocalBench.
> > > > >
> > > > >
> > > >  But it would be wrong since it opens a security vulnerability.
> > > >
> > > >  See:
> > > >  http://markmail.org/message/rwq3kif5n33wp65m
> > > >  http://svn.apache.org/viewvc?view=rev&revision=477355
> > > >
> > > >
> > > >  Regards,
> > > >  Tim
> > > >
> > > >
> > >
> > >
> >
>