You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by Aleksey Shipilev <al...@gmail.com> on 2008/04/24 17:45:27 UTC
[classlib][luni][performance] *HashMap cleanup and optimization (was: Re: [classlib][luni][performance] IdentityHashMap implementation)
Thanks, Alexey!
I see the plan is following:
1. Sweep the HashMap implementation and make the source beatuful: add
necessary comments, re-layout class members.
2. Test-commit-test sweeped HashMap implementation and see there are
no breakages.
3. Remove legacy IdentityHashMap and copy HashMap over it (using svn
capabilities)
4. Transform new IdentityHashMap to real IdentityHashMap (hashCode ->
identityHashCode, equals -> == and stuff)
5. Test-commit-test new IdentityHashMap.
Then we could think about generalization of IdentityHashMap and HashMap code.
Nathan, Mark, Alexey, do I summarize correct?
Thanks,
Aleksey.
On Wed, Apr 23, 2008 at 4:43 PM, Alexey Petrenko
<al...@gmail.com> wrote:
> Aleksey,
>
> I would support Mark here. Clear patch is very important because it
> makes our lives much easier and we are not required to skip 70K of
> irrelevant text to see 2 relevant lines :)
> So I think that your idea is good but should be better delivered :)
>
> Take a look at "Good Issue Resolution Guideline" for example.
> http://harmony.apache.org/issue_resolution_guideline.html
>
> SY, Alexey
>
> 2008/4/23, Mark Hindess <ma...@googlemail.com>:
>
>
> >
> > On 23 April 2008 at 0:36, "Aleksey Shipilev" <al...@gmail.com>
> > wrote:
> > > Hi Endre,
> > >
> > > On Tue, Apr 22, 2008 at 11:30 PM, Endre St=F8lsvik <En...@stolsvik.com> wro=
> > > te:
> > > > Aleksey Shipilev wrote:
> > > > > The reason behind all that changes is that entire IdentityHashMap
> > > > > implementation was thrown away and replaced by HashMap
> > > >
> > > > Isn't it possible to actually record this fact using SVN, by
> > > > deleting the file, and then adding it again (or svn copy it from
> > > > HashMap) - so that it doesn't look like a *change*, but more what it
> > > > actually is: a remove, and then an add (actually, a copy)?
> > >
> > > Unfortunately, that's not usable, you might play around to see why. If
> > > you find a solution, please let me know :)
> >
> > Fortunately, it is not compulsory to create patches with "svn diff".
> > I've just done:
> >
> > 1) apply your patch to a fresh checkout
> > 2) cp modules/luni/src/main/java/java/util/HashMap.java \
> > modules/luni/src/main/java/java/util/IdentityHashMap.java.orig
> > 3) diff -u modules/luni/src/main/java/java/util/IdentityHashMap.java.orig \
> > modules/luni/src/main/java/java/util/IdentityHashMap.java
> >
> > The resulting patch would be much more suitable for attachment to a JIRA.
> >
> > I'd still fix a few things about HashMap.java first though.
> > -Mark.
> >
> >
> >
>
Re: [classlib][luni][performance] *HashMap cleanup and optimization (was: Re: [classlib][luni][performance] IdentityHashMap implementation)
Posted by Aleksey Shipilev <al...@gmail.com>.
(friendly reminder)
Guys, can someone review HARMONY-5771?
This is IdentityHashMap migration to optimized HashMap code, giving
+20% to SPECjvm2008:serial.
Thanks,
Aleksey.
On Wed, May 21, 2008 at 6:37 PM, Aleksey Shipilev
<al...@gmail.com> wrote:
> Hi,
>
> Thanks to Mark, HARMONY-5791 (HashMap cleanup) was committed.
>
> Now I had moved IdentityHashMap implementation to be like cleaned
> HashMap and filed HARMONY-5771 with the patches. Could someone please
> review it?
>
> Thanks,
> Aleksey.
>
> On Wed, May 14, 2008 at 2:09 PM, Aleksey Shipilev
> <al...@gmail.com> wrote:
>> Hi,
>>
>> I'm back from my long vacation and see that milestone is almost ready,
>> so we could proceed with Collections works.
>> Can someone review HARMONY-5791?
>>
>> Thanks,
>> Aleksey.
>>
>> On Wed, Apr 30, 2008 at 12:19 PM, Aleksey Shipilev
>> <al...@gmail.com> wrote:
>>> Nathan,
>>>
>>> Is there an option to commit this patch before code freeze starts (if
>>> not already)?
>>> The changes there are pretty trivial and I could proceed with
>>> IdentityHashMap works while M6 is under testing.
>>>
>>> Thanks,
>>> Aleksey.
>>>
>>>
>>>
>>> On Wed, Apr 30, 2008 at 4:21 AM, Nathan Beyer <nd...@apache.org> wrote:
>>> > Bring this back up after the milestone is complete.
>>> >
>>> > -Nathan
>>> >
>>> > On Tue, Apr 29, 2008 at 1:49 AM, Aleksey Shipilev <
>>> >
>>> > aleksey.shipilev@gmail.com> wrote:
>>> >
>>> >
>>> > > Hi,
>>> > >
>>> > > > I see the plan is following:
>>> > > > 1. Sweep the HashMap implementation and make the source beatuful: add
>>> > > > necessary comments, re-layout class members.
>>> > > > 2. Test-commit-test sweeped HashMap implementation and see there are
>>> > > > no breakages.
>>> > > > 3. Remove legacy IdentityHashMap and copy HashMap over it (using svn
>>> > > > capabilities)
>>> > > > 4. Transform new IdentityHashMap to real IdentityHashMap (hashCode ->
>>> > > > identityHashCode, equals -> == and stuff)
>>> > > > 5. Test-commit-test new IdentityHashMap.
>>> > >
>>> > > It seems like we had stuck on (1) point on our plan, there is a patch
>>> > > already in HARMONY-5791, and we need to review it before moving
>>> > > towards actual performance works. LUNI/Collections gurus (Jimmy,
>>> > > Mark, Nathan, Tim?), please review it so I can proceed in my efforts.
>>> > > :)
>>> > >
>>> > > Thanks,
>>> > > Aleksey.
>>> > >
>>> > >
>>> > > On Mon, Apr 28, 2008 at 1:03 PM, Aleksey Shipilev
>>> > > <al...@gmail.com>>
>>> >
>>> > > wrote:
>>> > > > Hi again,
>>> > > >
>>> > > > Can some classlib guru review HARMONY-5791?
>>> > > >
>>> > > > Thanks,
>>> > > > Aleksey.
>>> > > >
>>> > > > On Fri, Apr 25, 2008 at 1:31 AM, Aleksey Shipilev
>>> > > >
>>> > > >
>>> > > > <al...@gmail.com>>
>>> >
>>> >
>>> > > wrote:
>>> > > > > Mark,
>>> > > > >
>>> > > > > That's great there are no regressions on Commons-collections tests!
>>> > > > > BTW, can we adopt them as the part of BTI or luni tests?
>>> > > > >
>>> > > > > I had created HARMONY-5791 for HashMap cleanup, and there's a first
>>> > > > > patch already, can you please take a look? I had extracted the
>>> > > > > contract-related methods there, so the change to IdentityHashMap
>>> > > > > should be pretty straightforward. After we finish with this issue, I
>>> > > > > could provide the clean script/patch for IdentityHashMap changes.
>>> > > > >
>>> > > > > Thanks,
>>> > > > > Aleksey.
>>> > > > >
>>> > > > >
>>> > > > >
>>> > >
>>> >
>>>
>>
>
Re: [classlib][luni][performance] *HashMap cleanup and optimization (was: Re: [classlib][luni][performance] IdentityHashMap implementation)
Posted by Aleksey Shipilev <al...@gmail.com>.
Hi,
Thanks to Mark, HARMONY-5791 (HashMap cleanup) was committed.
Now I had moved IdentityHashMap implementation to be like cleaned
HashMap and filed HARMONY-5771 with the patches. Could someone please
review it?
Thanks,
Aleksey.
On Wed, May 14, 2008 at 2:09 PM, Aleksey Shipilev
<al...@gmail.com> wrote:
> Hi,
>
> I'm back from my long vacation and see that milestone is almost ready,
> so we could proceed with Collections works.
> Can someone review HARMONY-5791?
>
> Thanks,
> Aleksey.
>
> On Wed, Apr 30, 2008 at 12:19 PM, Aleksey Shipilev
> <al...@gmail.com> wrote:
>> Nathan,
>>
>> Is there an option to commit this patch before code freeze starts (if
>> not already)?
>> The changes there are pretty trivial and I could proceed with
>> IdentityHashMap works while M6 is under testing.
>>
>> Thanks,
>> Aleksey.
>>
>>
>>
>> On Wed, Apr 30, 2008 at 4:21 AM, Nathan Beyer <nd...@apache.org> wrote:
>> > Bring this back up after the milestone is complete.
>> >
>> > -Nathan
>> >
>> > On Tue, Apr 29, 2008 at 1:49 AM, Aleksey Shipilev <
>> >
>> > aleksey.shipilev@gmail.com> wrote:
>> >
>> >
>> > > Hi,
>> > >
>> > > > I see the plan is following:
>> > > > 1. Sweep the HashMap implementation and make the source beatuful: add
>> > > > necessary comments, re-layout class members.
>> > > > 2. Test-commit-test sweeped HashMap implementation and see there are
>> > > > no breakages.
>> > > > 3. Remove legacy IdentityHashMap and copy HashMap over it (using svn
>> > > > capabilities)
>> > > > 4. Transform new IdentityHashMap to real IdentityHashMap (hashCode ->
>> > > > identityHashCode, equals -> == and stuff)
>> > > > 5. Test-commit-test new IdentityHashMap.
>> > >
>> > > It seems like we had stuck on (1) point on our plan, there is a patch
>> > > already in HARMONY-5791, and we need to review it before moving
>> > > towards actual performance works. LUNI/Collections gurus (Jimmy,
>> > > Mark, Nathan, Tim?), please review it so I can proceed in my efforts.
>> > > :)
>> > >
>> > > Thanks,
>> > > Aleksey.
>> > >
>> > >
>> > > On Mon, Apr 28, 2008 at 1:03 PM, Aleksey Shipilev
>> > > <al...@gmail.com>>
>> >
>> > > wrote:
>> > > > Hi again,
>> > > >
>> > > > Can some classlib guru review HARMONY-5791?
>> > > >
>> > > > Thanks,
>> > > > Aleksey.
>> > > >
>> > > > On Fri, Apr 25, 2008 at 1:31 AM, Aleksey Shipilev
>> > > >
>> > > >
>> > > > <al...@gmail.com>>
>> >
>> >
>> > > wrote:
>> > > > > Mark,
>> > > > >
>> > > > > That's great there are no regressions on Commons-collections tests!
>> > > > > BTW, can we adopt them as the part of BTI or luni tests?
>> > > > >
>> > > > > I had created HARMONY-5791 for HashMap cleanup, and there's a first
>> > > > > patch already, can you please take a look? I had extracted the
>> > > > > contract-related methods there, so the change to IdentityHashMap
>> > > > > should be pretty straightforward. After we finish with this issue, I
>> > > > > could provide the clean script/patch for IdentityHashMap changes.
>> > > > >
>> > > > > Thanks,
>> > > > > Aleksey.
>> > > > >
>> > > > >
>> > > > >
>> > >
>> >
>>
>
Re: [classlib][luni][performance] *HashMap cleanup and optimization (was: Re: [classlib][luni][performance] IdentityHashMap implementation)
Posted by Aleksey Shipilev <al...@gmail.com>.
Hi,
I'm back from my long vacation and see that milestone is almost ready,
so we could proceed with Collections works.
Can someone review HARMONY-5791?
Thanks,
Aleksey.
On Wed, Apr 30, 2008 at 12:19 PM, Aleksey Shipilev
<al...@gmail.com> wrote:
> Nathan,
>
> Is there an option to commit this patch before code freeze starts (if
> not already)?
> The changes there are pretty trivial and I could proceed with
> IdentityHashMap works while M6 is under testing.
>
> Thanks,
> Aleksey.
>
>
>
> On Wed, Apr 30, 2008 at 4:21 AM, Nathan Beyer <nd...@apache.org> wrote:
> > Bring this back up after the milestone is complete.
> >
> > -Nathan
> >
> > On Tue, Apr 29, 2008 at 1:49 AM, Aleksey Shipilev <
> >
> > aleksey.shipilev@gmail.com> wrote:
> >
> >
> > > Hi,
> > >
> > > > I see the plan is following:
> > > > 1. Sweep the HashMap implementation and make the source beatuful: add
> > > > necessary comments, re-layout class members.
> > > > 2. Test-commit-test sweeped HashMap implementation and see there are
> > > > no breakages.
> > > > 3. Remove legacy IdentityHashMap and copy HashMap over it (using svn
> > > > capabilities)
> > > > 4. Transform new IdentityHashMap to real IdentityHashMap (hashCode ->
> > > > identityHashCode, equals -> == and stuff)
> > > > 5. Test-commit-test new IdentityHashMap.
> > >
> > > It seems like we had stuck on (1) point on our plan, there is a patch
> > > already in HARMONY-5791, and we need to review it before moving
> > > towards actual performance works. LUNI/Collections gurus (Jimmy,
> > > Mark, Nathan, Tim?), please review it so I can proceed in my efforts.
> > > :)
> > >
> > > Thanks,
> > > Aleksey.
> > >
> > >
> > > On Mon, Apr 28, 2008 at 1:03 PM, Aleksey Shipilev
> > > <al...@gmail.com>>
> >
> > > wrote:
> > > > Hi again,
> > > >
> > > > Can some classlib guru review HARMONY-5791?
> > > >
> > > > Thanks,
> > > > Aleksey.
> > > >
> > > > On Fri, Apr 25, 2008 at 1:31 AM, Aleksey Shipilev
> > > >
> > > >
> > > > <al...@gmail.com>>
> >
> >
> > > wrote:
> > > > > Mark,
> > > > >
> > > > > That's great there are no regressions on Commons-collections tests!
> > > > > BTW, can we adopt them as the part of BTI or luni tests?
> > > > >
> > > > > I had created HARMONY-5791 for HashMap cleanup, and there's a first
> > > > > patch already, can you please take a look? I had extracted the
> > > > > contract-related methods there, so the change to IdentityHashMap
> > > > > should be pretty straightforward. After we finish with this issue, I
> > > > > could provide the clean script/patch for IdentityHashMap changes.
> > > > >
> > > > > Thanks,
> > > > > Aleksey.
> > > > >
> > > > >
> > > > >
> > >
> >
>
Re: [classlib][luni][performance] *HashMap cleanup and optimization
Posted by Mark Hindess <ma...@googlemail.com>.
In message <4b...@mail.gmail.com>,
"Aleksey Shipilev" writes:
>
> Nathan,
>
> Is there an option to commit this patch before code freeze starts (if
> not already)?
It has started so non-bugfix code changes must wait. Sorry. (I've
been busy with some other things and svn write access has been broken
anyway.)
> The changes there are pretty trivial and I could proceed with
> IdentityHashMap works while M6 is under testing.
You might want to consider looking at other tools such as git[0]/quilt
to enable you to continue testing new patches without waiting for
existing patches to be committed.
Or if you prefer you could work on bug fixing for a week or two.
-Mark.
[0] Salikh Zakirov's message about git is here:
http://markmail.org/message/zypnc4jv7xaz7nqu
Re: [classlib][luni][performance] *HashMap cleanup and optimization (was: Re: [classlib][luni][performance] IdentityHashMap implementation)
Posted by Aleksey Shipilev <al...@gmail.com>.
Nathan,
Is there an option to commit this patch before code freeze starts (if
not already)?
The changes there are pretty trivial and I could proceed with
IdentityHashMap works while M6 is under testing.
Thanks,
Aleksey.
On Wed, Apr 30, 2008 at 4:21 AM, Nathan Beyer <nd...@apache.org> wrote:
> Bring this back up after the milestone is complete.
>
> -Nathan
>
> On Tue, Apr 29, 2008 at 1:49 AM, Aleksey Shipilev <
>
> aleksey.shipilev@gmail.com> wrote:
>
>
> > Hi,
> >
> > > I see the plan is following:
> > > 1. Sweep the HashMap implementation and make the source beatuful: add
> > > necessary comments, re-layout class members.
> > > 2. Test-commit-test sweeped HashMap implementation and see there are
> > > no breakages.
> > > 3. Remove legacy IdentityHashMap and copy HashMap over it (using svn
> > > capabilities)
> > > 4. Transform new IdentityHashMap to real IdentityHashMap (hashCode ->
> > > identityHashCode, equals -> == and stuff)
> > > 5. Test-commit-test new IdentityHashMap.
> >
> > It seems like we had stuck on (1) point on our plan, there is a patch
> > already in HARMONY-5791, and we need to review it before moving
> > towards actual performance works. LUNI/Collections gurus (Jimmy,
> > Mark, Nathan, Tim?), please review it so I can proceed in my efforts.
> > :)
> >
> > Thanks,
> > Aleksey.
> >
> >
> > On Mon, Apr 28, 2008 at 1:03 PM, Aleksey Shipilev
> > <al...@gmail.com>>
>
> > wrote:
> > > Hi again,
> > >
> > > Can some classlib guru review HARMONY-5791?
> > >
> > > Thanks,
> > > Aleksey.
> > >
> > > On Fri, Apr 25, 2008 at 1:31 AM, Aleksey Shipilev
> > >
> > >
> > > <al...@gmail.com>>
>
>
> > wrote:
> > > > Mark,
> > > >
> > > > That's great there are no regressions on Commons-collections tests!
> > > > BTW, can we adopt them as the part of BTI or luni tests?
> > > >
> > > > I had created HARMONY-5791 for HashMap cleanup, and there's a first
> > > > patch already, can you please take a look? I had extracted the
> > > > contract-related methods there, so the change to IdentityHashMap
> > > > should be pretty straightforward. After we finish with this issue, I
> > > > could provide the clean script/patch for IdentityHashMap changes.
> > > >
> > > > Thanks,
> > > > Aleksey.
> > > >
> > > >
> > > >
> >
>
Re: [classlib][luni][performance] *HashMap cleanup and optimization (was: Re: [classlib][luni][performance] IdentityHashMap implementation)
Posted by Nathan Beyer <nd...@apache.org>.
Bring this back up after the milestone is complete.
-Nathan
On Tue, Apr 29, 2008 at 1:49 AM, Aleksey Shipilev <
aleksey.shipilev@gmail.com> wrote:
> Hi,
>
> > I see the plan is following:
> > 1. Sweep the HashMap implementation and make the source beatuful: add
> > necessary comments, re-layout class members.
> > 2. Test-commit-test sweeped HashMap implementation and see there are
> > no breakages.
> > 3. Remove legacy IdentityHashMap and copy HashMap over it (using svn
> > capabilities)
> > 4. Transform new IdentityHashMap to real IdentityHashMap (hashCode ->
> > identityHashCode, equals -> == and stuff)
> > 5. Test-commit-test new IdentityHashMap.
>
> It seems like we had stuck on (1) point on our plan, there is a patch
> already in HARMONY-5791, and we need to review it before moving
> towards actual performance works. LUNI/Collections gurus (Jimmy,
> Mark, Nathan, Tim?), please review it so I can proceed in my efforts.
> :)
>
> Thanks,
> Aleksey.
>
>
> On Mon, Apr 28, 2008 at 1:03 PM, Aleksey Shipilev
> <al...@gmail.com>>
> wrote:
> > Hi again,
> >
> > Can some classlib guru review HARMONY-5791?
> >
> > Thanks,
> > Aleksey.
> >
> > On Fri, Apr 25, 2008 at 1:31 AM, Aleksey Shipilev
> >
> >
> > <al...@gmail.com>>
> wrote:
> > > Mark,
> > >
> > > That's great there are no regressions on Commons-collections tests!
> > > BTW, can we adopt them as the part of BTI or luni tests?
> > >
> > > I had created HARMONY-5791 for HashMap cleanup, and there's a first
> > > patch already, can you please take a look? I had extracted the
> > > contract-related methods there, so the change to IdentityHashMap
> > > should be pretty straightforward. After we finish with this issue, I
> > > could provide the clean script/patch for IdentityHashMap changes.
> > >
> > > Thanks,
> > > Aleksey.
> > >
> > >
> > >
>
Re: [classlib][luni][performance] *HashMap cleanup and optimization (was: Re: [classlib][luni][performance] IdentityHashMap implementation)
Posted by Aleksey Shipilev <al...@gmail.com>.
Hi,
> I see the plan is following:
> 1. Sweep the HashMap implementation and make the source beatuful: add
> necessary comments, re-layout class members.
> 2. Test-commit-test sweeped HashMap implementation and see there are
> no breakages.
> 3. Remove legacy IdentityHashMap and copy HashMap over it (using svn
> capabilities)
> 4. Transform new IdentityHashMap to real IdentityHashMap (hashCode ->
> identityHashCode, equals -> == and stuff)
> 5. Test-commit-test new IdentityHashMap.
It seems like we had stuck on (1) point on our plan, there is a patch
already in HARMONY-5791, and we need to review it before moving
towards actual performance works. LUNI/Collections gurus (Jimmy,
Mark, Nathan, Tim?), please review it so I can proceed in my efforts.
:)
Thanks,
Aleksey.
On Mon, Apr 28, 2008 at 1:03 PM, Aleksey Shipilev
<al...@gmail.com> wrote:
> Hi again,
>
> Can some classlib guru review HARMONY-5791?
>
> Thanks,
> Aleksey.
>
> On Fri, Apr 25, 2008 at 1:31 AM, Aleksey Shipilev
>
>
> <al...@gmail.com> wrote:
> > Mark,
> >
> > That's great there are no regressions on Commons-collections tests!
> > BTW, can we adopt them as the part of BTI or luni tests?
> >
> > I had created HARMONY-5791 for HashMap cleanup, and there's a first
> > patch already, can you please take a look? I had extracted the
> > contract-related methods there, so the change to IdentityHashMap
> > should be pretty straightforward. After we finish with this issue, I
> > could provide the clean script/patch for IdentityHashMap changes.
> >
> > Thanks,
> > Aleksey.
> >
> >
> >
Re: [classlib][luni][performance] *HashMap cleanup and optimization (was: Re: [classlib][luni][performance] IdentityHashMap implementation)
Posted by Aleksey Shipilev <al...@gmail.com>.
Hi again,
Can some classlib guru review HARMONY-5791?
Thanks,
Aleksey.
On Fri, Apr 25, 2008 at 1:31 AM, Aleksey Shipilev
<al...@gmail.com> wrote:
> Mark,
>
> That's great there are no regressions on Commons-collections tests!
> BTW, can we adopt them as the part of BTI or luni tests?
>
> I had created HARMONY-5791 for HashMap cleanup, and there's a first
> patch already, can you please take a look? I had extracted the
> contract-related methods there, so the change to IdentityHashMap
> should be pretty straightforward. After we finish with this issue, I
> could provide the clean script/patch for IdentityHashMap changes.
>
> Thanks,
> Aleksey.
>
>
>
> On Thu, Apr 24, 2008 at 11:31 PM, Mark Hindess
> <ma...@googlemail.com> wrote:
> > +1
> >
> > Incidentally, I tested your previous patch and it looks good to me. I
> > also tried running the commons-collections tests - thinking that these
> > might be a good way to achieve better coverage - and they also pass
> > except for one unrelated failure (HARMONY-5788).
> >
> > I've not had chance to look at the performance implications yet.
> >
> > -Mark.
> >
> > [0] http://commons.apache.org/collections/
> >
> > In message <4b...@mail.gmail.com>,
> >
> >
> > "Aleksey Shipilev" writes:
> > >
> > > Thanks, Alexey!
> > >
> > > I see the plan is following:
> > > 1. Sweep the HashMap implementation and make the source beatuful: add
> > > necessary comments, re-layout class members.
> > > 2. Test-commit-test sweeped HashMap implementation and see there are
> > > no breakages.
> > > 3. Remove legacy IdentityHashMap and copy HashMap over it (using svn
> > > capabilities)
> > > 4. Transform new IdentityHashMap to real IdentityHashMap (hashCode ->
> > > identityHashCode, equals -> == and stuff)
> > > 5. Test-commit-test new IdentityHashMap.
> > >
> > > Then we could think about generalization of IdentityHashMap and HashMap code.
> > >
> > > Nathan, Mark, Alexey, do I summarize correct?
> > >
> > > Thanks,
> > > Aleksey.
> > >
> > > On Wed, Apr 23, 2008 at 4:43 PM, Alexey Petrenko
> > > <al...@gmail.com> wrote:
> > > > Aleksey,
> > > >
> > > > I would support Mark here. Clear patch is very important because it
> > > > makes our lives much easier and we are not required to skip 70K of
> > > > irrelevant text to see 2 relevant lines :)
> > > > So I think that your idea is good but should be better delivered :)
> > > >
> > > > Take a look at "Good Issue Resolution Guideline" for example.
> > > > http://harmony.apache.org/issue_resolution_guideline.html
> > > >
> > > > SY, Alexey
> > > >
> > > > 2008/4/23, Mark Hindess <ma...@googlemail.com>:
> > > >
> > > >
> > > > >
> > > > > On 23 April 2008 at 0:36, "Aleksey Shipilev" <aleksey.shipilev@gmail.com
> > > >
> > > > > wrote:
> > > > > > Hi Endre,
> > > > > >
> > > > > > On Tue, Apr 22, 2008 at 11:30 PM, Endre St=F8lsvik <Endre@stolsvik.com
> > > > wro=
> > > > > > te:
> > > > > > > Aleksey Shipilev wrote:
> > > > > > > > The reason behind all that changes is that entire IdentityHashMap
> > > > > > > > implementation was thrown away and replaced by HashMap
> > > > > > >
> > > > > > > Isn't it possible to actually record this fact using SVN, by
> > > > > > > deleting the file, and then adding it again (or svn copy it from
> > > > > > > HashMap) - so that it doesn't look like a *change*, but more what it
> > > > > > > actually is: a remove, and then an add (actually, a copy)?
> > > > > >
> > > > > > Unfortunately, that's not usable, you might play around to see why. If
> > > > > > you find a solution, please let me know :)
> > > > >
> > > > > Fortunately, it is not compulsory to create patches with "svn diff".
> > > > > I've just done:
> > > > >
> > > > > 1) apply your patch to a fresh checkout
> > > > > 2) cp modules/luni/src/main/java/java/util/HashMap.java \
> > > > > modules/luni/src/main/java/java/util/IdentityHashMap.java.orig
> > > > > 3) diff -u modules/luni/src/main/java/java/util/IdentityHashMap.java.ori
> > > g \
> > > > > modules/luni/src/main/java/java/util/IdentityHashMap.java
> > > > >
> > > > > The resulting patch would be much more suitable for attachment to a JIRA
> > > .
> > > > >
> > > > > I'd still fix a few things about HashMap.java first though.
> > > > > -Mark.
> > > > >
> > > > >
> > > > >
> > > >
> > >
> >
> >
> >
>
Re: [classlib][luni][performance] *HashMap cleanup and optimization (was: Re: [classlib][luni][performance] IdentityHashMap implementation)
Posted by Alexei Fedotov <al...@gmail.com>.
Aleksey,
I believe the proper place for external tests is BTI. An adapter is welcome. :-)
Thanks!
On Fri, Apr 25, 2008 at 1:31 AM, Aleksey Shipilev
<al...@gmail.com> wrote:
> Mark,
>
> That's great there are no regressions on Commons-collections tests!
> BTW, can we adopt them as the part of BTI or luni tests?
>
> I had created HARMONY-5791 for HashMap cleanup, and there's a first
> patch already, can you please take a look? I had extracted the
> contract-related methods there, so the change to IdentityHashMap
> should be pretty straightforward. After we finish with this issue, I
> could provide the clean script/patch for IdentityHashMap changes.
>
> Thanks,
> Aleksey.
>
>
>
> On Thu, Apr 24, 2008 at 11:31 PM, Mark Hindess
> <ma...@googlemail.com> wrote:
> > +1
> >
> > Incidentally, I tested your previous patch and it looks good to me. I
> > also tried running the commons-collections tests - thinking that these
> > might be a good way to achieve better coverage - and they also pass
> > except for one unrelated failure (HARMONY-5788).
> >
> > I've not had chance to look at the performance implications yet.
> >
> > -Mark.
> >
> > [0] http://commons.apache.org/collections/
> >
> > In message <4b...@mail.gmail.com>,
> >
> >
> > "Aleksey Shipilev" writes:
> > >
> > > Thanks, Alexey!
> > >
> > > I see the plan is following:
> > > 1. Sweep the HashMap implementation and make the source beatuful: add
> > > necessary comments, re-layout class members.
> > > 2. Test-commit-test sweeped HashMap implementation and see there are
> > > no breakages.
> > > 3. Remove legacy IdentityHashMap and copy HashMap over it (using svn
> > > capabilities)
> > > 4. Transform new IdentityHashMap to real IdentityHashMap (hashCode ->
> > > identityHashCode, equals -> == and stuff)
> > > 5. Test-commit-test new IdentityHashMap.
> > >
> > > Then we could think about generalization of IdentityHashMap and HashMap code.
> > >
> > > Nathan, Mark, Alexey, do I summarize correct?
> > >
> > > Thanks,
> > > Aleksey.
> > >
> > > On Wed, Apr 23, 2008 at 4:43 PM, Alexey Petrenko
> > > <al...@gmail.com> wrote:
> > > > Aleksey,
> > > >
> > > > I would support Mark here. Clear patch is very important because it
> > > > makes our lives much easier and we are not required to skip 70K of
> > > > irrelevant text to see 2 relevant lines :)
> > > > So I think that your idea is good but should be better delivered :)
> > > >
> > > > Take a look at "Good Issue Resolution Guideline" for example.
> > > > http://harmony.apache.org/issue_resolution_guideline.html
> > > >
> > > > SY, Alexey
> > > >
> > > > 2008/4/23, Mark Hindess <ma...@googlemail.com>:
> > > >
> > > >
> > > > >
> > > > > On 23 April 2008 at 0:36, "Aleksey Shipilev" <aleksey.shipilev@gmail.com
> > > >
> > > > > wrote:
> > > > > > Hi Endre,
> > > > > >
> > > > > > On Tue, Apr 22, 2008 at 11:30 PM, Endre St=F8lsvik <Endre@stolsvik.com
> > > > wro=
> > > > > > te:
> > > > > > > Aleksey Shipilev wrote:
> > > > > > > > The reason behind all that changes is that entire IdentityHashMap
> > > > > > > > implementation was thrown away and replaced by HashMap
> > > > > > >
> > > > > > > Isn't it possible to actually record this fact using SVN, by
> > > > > > > deleting the file, and then adding it again (or svn copy it from
> > > > > > > HashMap) - so that it doesn't look like a *change*, but more what it
> > > > > > > actually is: a remove, and then an add (actually, a copy)?
> > > > > >
> > > > > > Unfortunately, that's not usable, you might play around to see why. If
> > > > > > you find a solution, please let me know :)
> > > > >
> > > > > Fortunately, it is not compulsory to create patches with "svn diff".
> > > > > I've just done:
> > > > >
> > > > > 1) apply your patch to a fresh checkout
> > > > > 2) cp modules/luni/src/main/java/java/util/HashMap.java \
> > > > > modules/luni/src/main/java/java/util/IdentityHashMap.java.orig
> > > > > 3) diff -u modules/luni/src/main/java/java/util/IdentityHashMap.java.ori
> > > g \
> > > > > modules/luni/src/main/java/java/util/IdentityHashMap.java
> > > > >
> > > > > The resulting patch would be much more suitable for attachment to a JIRA
> > > .
> > > > >
> > > > > I'd still fix a few things about HashMap.java first though.
> > > > > -Mark.
> > > > >
> > > > >
> > > > >
> > > >
> > >
> >
> >
> >
>
--
With best regards,
Alexei
Re: [classlib][luni][performance] *HashMap cleanup and optimization (was: Re: [classlib][luni][performance] IdentityHashMap implementation)
Posted by Aleksey Shipilev <al...@gmail.com>.
Mark,
That's great there are no regressions on Commons-collections tests!
BTW, can we adopt them as the part of BTI or luni tests?
I had created HARMONY-5791 for HashMap cleanup, and there's a first
patch already, can you please take a look? I had extracted the
contract-related methods there, so the change to IdentityHashMap
should be pretty straightforward. After we finish with this issue, I
could provide the clean script/patch for IdentityHashMap changes.
Thanks,
Aleksey.
On Thu, Apr 24, 2008 at 11:31 PM, Mark Hindess
<ma...@googlemail.com> wrote:
> +1
>
> Incidentally, I tested your previous patch and it looks good to me. I
> also tried running the commons-collections tests - thinking that these
> might be a good way to achieve better coverage - and they also pass
> except for one unrelated failure (HARMONY-5788).
>
> I've not had chance to look at the performance implications yet.
>
> -Mark.
>
> [0] http://commons.apache.org/collections/
>
> In message <4b...@mail.gmail.com>,
>
>
> "Aleksey Shipilev" writes:
> >
> > Thanks, Alexey!
> >
> > I see the plan is following:
> > 1. Sweep the HashMap implementation and make the source beatuful: add
> > necessary comments, re-layout class members.
> > 2. Test-commit-test sweeped HashMap implementation and see there are
> > no breakages.
> > 3. Remove legacy IdentityHashMap and copy HashMap over it (using svn
> > capabilities)
> > 4. Transform new IdentityHashMap to real IdentityHashMap (hashCode ->
> > identityHashCode, equals -> == and stuff)
> > 5. Test-commit-test new IdentityHashMap.
> >
> > Then we could think about generalization of IdentityHashMap and HashMap code.
> >
> > Nathan, Mark, Alexey, do I summarize correct?
> >
> > Thanks,
> > Aleksey.
> >
> > On Wed, Apr 23, 2008 at 4:43 PM, Alexey Petrenko
> > <al...@gmail.com> wrote:
> > > Aleksey,
> > >
> > > I would support Mark here. Clear patch is very important because it
> > > makes our lives much easier and we are not required to skip 70K of
> > > irrelevant text to see 2 relevant lines :)
> > > So I think that your idea is good but should be better delivered :)
> > >
> > > Take a look at "Good Issue Resolution Guideline" for example.
> > > http://harmony.apache.org/issue_resolution_guideline.html
> > >
> > > SY, Alexey
> > >
> > > 2008/4/23, Mark Hindess <ma...@googlemail.com>:
> > >
> > >
> > > >
> > > > On 23 April 2008 at 0:36, "Aleksey Shipilev" <aleksey.shipilev@gmail.com
> > >
> > > > wrote:
> > > > > Hi Endre,
> > > > >
> > > > > On Tue, Apr 22, 2008 at 11:30 PM, Endre St=F8lsvik <Endre@stolsvik.com
> > > wro=
> > > > > te:
> > > > > > Aleksey Shipilev wrote:
> > > > > > > The reason behind all that changes is that entire IdentityHashMap
> > > > > > > implementation was thrown away and replaced by HashMap
> > > > > >
> > > > > > Isn't it possible to actually record this fact using SVN, by
> > > > > > deleting the file, and then adding it again (or svn copy it from
> > > > > > HashMap) - so that it doesn't look like a *change*, but more what it
> > > > > > actually is: a remove, and then an add (actually, a copy)?
> > > > >
> > > > > Unfortunately, that's not usable, you might play around to see why. If
> > > > > you find a solution, please let me know :)
> > > >
> > > > Fortunately, it is not compulsory to create patches with "svn diff".
> > > > I've just done:
> > > >
> > > > 1) apply your patch to a fresh checkout
> > > > 2) cp modules/luni/src/main/java/java/util/HashMap.java \
> > > > modules/luni/src/main/java/java/util/IdentityHashMap.java.orig
> > > > 3) diff -u modules/luni/src/main/java/java/util/IdentityHashMap.java.ori
> > g \
> > > > modules/luni/src/main/java/java/util/IdentityHashMap.java
> > > >
> > > > The resulting patch would be much more suitable for attachment to a JIRA
> > .
> > > >
> > > > I'd still fix a few things about HashMap.java first though.
> > > > -Mark.
> > > >
> > > >
> > > >
> > >
> >
>
>
>
Re: [classlib][luni][performance] *HashMap cleanup and optimization
(was: Re: [classlib][luni][performance] IdentityHashMap implementation)
Posted by Mark Hindess <ma...@googlemail.com>.
+1
Incidentally, I tested your previous patch and it looks good to me. I
also tried running the commons-collections tests - thinking that these
might be a good way to achieve better coverage - and they also pass
except for one unrelated failure (HARMONY-5788).
I've not had chance to look at the performance implications yet.
-Mark.
[0] http://commons.apache.org/collections/
In message <4b...@mail.gmail.com>,
"Aleksey Shipilev" writes:
>
> Thanks, Alexey!
>
> I see the plan is following:
> 1. Sweep the HashMap implementation and make the source beatuful: add
> necessary comments, re-layout class members.
> 2. Test-commit-test sweeped HashMap implementation and see there are
> no breakages.
> 3. Remove legacy IdentityHashMap and copy HashMap over it (using svn
> capabilities)
> 4. Transform new IdentityHashMap to real IdentityHashMap (hashCode ->
> identityHashCode, equals -> == and stuff)
> 5. Test-commit-test new IdentityHashMap.
>
> Then we could think about generalization of IdentityHashMap and HashMap code.
>
> Nathan, Mark, Alexey, do I summarize correct?
>
> Thanks,
> Aleksey.
>
> On Wed, Apr 23, 2008 at 4:43 PM, Alexey Petrenko
> <al...@gmail.com> wrote:
> > Aleksey,
> >
> > I would support Mark here. Clear patch is very important because it
> > makes our lives much easier and we are not required to skip 70K of
> > irrelevant text to see 2 relevant lines :)
> > So I think that your idea is good but should be better delivered :)
> >
> > Take a look at "Good Issue Resolution Guideline" for example.
> > http://harmony.apache.org/issue_resolution_guideline.html
> >
> > SY, Alexey
> >
> > 2008/4/23, Mark Hindess <ma...@googlemail.com>:
> >
> >
> > >
> > > On 23 April 2008 at 0:36, "Aleksey Shipilev" <aleksey.shipilev@gmail.com
> >
> > > wrote:
> > > > Hi Endre,
> > > >
> > > > On Tue, Apr 22, 2008 at 11:30 PM, Endre St=F8lsvik <Endre@stolsvik.com
> > wro=
> > > > te:
> > > > > Aleksey Shipilev wrote:
> > > > > > The reason behind all that changes is that entire IdentityHashMap
> > > > > > implementation was thrown away and replaced by HashMap
> > > > >
> > > > > Isn't it possible to actually record this fact using SVN, by
> > > > > deleting the file, and then adding it again (or svn copy it from
> > > > > HashMap) - so that it doesn't look like a *change*, but more what it
> > > > > actually is: a remove, and then an add (actually, a copy)?
> > > >
> > > > Unfortunately, that's not usable, you might play around to see why. If
> > > > you find a solution, please let me know :)
> > >
> > > Fortunately, it is not compulsory to create patches with "svn diff".
> > > I've just done:
> > >
> > > 1) apply your patch to a fresh checkout
> > > 2) cp modules/luni/src/main/java/java/util/HashMap.java \
> > > modules/luni/src/main/java/java/util/IdentityHashMap.java.orig
> > > 3) diff -u modules/luni/src/main/java/java/util/IdentityHashMap.java.ori
> g \
> > > modules/luni/src/main/java/java/util/IdentityHashMap.java
> > >
> > > The resulting patch would be much more suitable for attachment to a JIRA
> .
> > >
> > > I'd still fix a few things about HashMap.java first though.
> > > -Mark.
> > >
> > >
> > >
> >
>