You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by Tim Ellison <t....@gmail.com> on 2007/12/07 15:02:54 UTC
[classlib][luni] TreeMap woes (was: Re: [jira] Commented: (HARMONY-5265)
[classlib][luni] TreeMap bug fixing and further performance improvements)
This change to TreeMap seems to have introduced an instability, and I
would say it is a candidate for reverting in M4.
WDYT?
Regards,
Tim
Ilya Berezhniuk (JIRA) wrote:
> [ https://issues.apache.org/jira/browse/HARMONY-5265?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12549389 ]
>
> Ilya Berezhniuk commented on HARMONY-5265:
> ------------------------------------------
>
> I've checked 2 times with debug build, crash does not appear.
> But 3 unexpected failures are still here.
>
>> [classlib][luni] TreeMap bug fixing and further performance improvements
>> ------------------------------------------------------------------------
>>
>> Key: HARMONY-5265
>> URL: https://issues.apache.org/jira/browse/HARMONY-5265
>> Project: Harmony
>> Issue Type: Bug
>> Components: Classlib
>> Affects Versions: 5.0M4
>> Reporter: Sergey Kuksenko
>> Assignee: Alexey Petrenko
>> Fix For: 5.0M4
>>
>> Attachments: TreeMap64Rv7.patch
>>
>>
>> Attached patch is further development of idea shown in HARMONY-5232.
>> As it was obtained using sorted arrays for small trees gives a boost over classical trees.
>> "In case of small SortedMap storing data in sorted array leads to significant increasing of cache hits. Practical measurements shows that operation get() became 10-30% faster and operation put is 3-10% faster. Even additional overhead for moving data for put() operation is smaller then obtained benefit and as result we got speedup."
>> HARMONY-5232 splitted internal tree representation into two parts: sorted array for small tree and classical tree for all other cases.
>> The attached patch implements the idea that we can store the origival tree structure, but instead of storing one key in the tree node (classical tree) each tree node contains small sorted array of keys. As result we:
>> - utilize performance benefits from "small sorted array" not only for small trees (as it was done in HARMONY-5232), but for all trees.
>> - avoid internal dynamic switching between two kinds of internal tree representation and makes code simple.
>> The latest fact has a big advantage for bug fixing, so fixing bugs described in HARMONY-5248 is much easier.
>> The current patch fixes all problems with EUT (HARMONY-5248).
>
Re: [classlib][luni] TreeMap woes (was: Re: [jira] Commented: (HARMONY-5265) [classlib][luni] TreeMap bug fixing and further performance improvements)
Posted by Alexey Petrenko <al...@gmail.com>.
This patch gives us significant boost.
So it's better to investigate, fix and keep it in M4 then revert.
We still have some time before testing cycle and Sergey is investigating.
Be patient :)
SY, Alexey
2007/12/7, Tim Ellison <t....@gmail.com>:
> This change to TreeMap seems to have introduced an instability, and I
> would say it is a candidate for reverting in M4.
>
> WDYT?
>
> Regards,
> Tim
>
> Ilya Berezhniuk (JIRA) wrote:
> > [ https://issues.apache.org/jira/browse/HARMONY-5265?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12549389 ]
> >
> > Ilya Berezhniuk commented on HARMONY-5265:
> > ------------------------------------------
> >
> > I've checked 2 times with debug build, crash does not appear.
> > But 3 unexpected failures are still here.
> >
> >> [classlib][luni] TreeMap bug fixing and further performance improvements
> >> ------------------------------------------------------------------------
> >>
> >> Key: HARMONY-5265
> >> URL: https://issues.apache.org/jira/browse/HARMONY-5265
> >> Project: Harmony
> >> Issue Type: Bug
> >> Components: Classlib
> >> Affects Versions: 5.0M4
> >> Reporter: Sergey Kuksenko
> >> Assignee: Alexey Petrenko
> >> Fix For: 5.0M4
> >>
> >> Attachments: TreeMap64Rv7.patch
> >>
> >>
> >> Attached patch is further development of idea shown in HARMONY-5232.
> >> As it was obtained using sorted arrays for small trees gives a boost over classical trees.
> >> "In case of small SortedMap storing data in sorted array leads to significant increasing of cache hits. Practical measurements shows that operation get() became 10-30% faster and operation put is 3-10% faster. Even additional overhead for moving data for put() operation is smaller then obtained benefit and as result we got speedup."
> >> HARMONY-5232 splitted internal tree representation into two parts: sorted array for small tree and classical tree for all other cases.
> >> The attached patch implements the idea that we can store the origival tree structure, but instead of storing one key in the tree node (classical tree) each tree node contains small sorted array of keys. As result we:
> >> - utilize performance benefits from "small sorted array" not only for small trees (as it was done in HARMONY-5232), but for all trees.
> >> - avoid internal dynamic switching between two kinds of internal tree representation and makes code simple.
> >> The latest fact has a big advantage for bug fixing, so fixing bugs described in HARMONY-5248 is much easier.
> >> The current patch fixes all problems with EUT (HARMONY-5248).
> >
>
Re: [classlib][luni] TreeMap woes
Posted by Nathan Beyer <nb...@gmail.com>.
The lesson - Don't check in enhancements, optimizations or anything
that's absolutely essential during a code freeze.
There was agreement to freeze, but this code made it in even though it
wasn't a critical fix.
-Nathan
On Dec 8, 2007 3:20 PM, Tim Ellison <t....@gmail.com> wrote:
> Nathan Beyer wrote:
> > It depends. Have we learned our lesson?
>
> What lesson is that?
> (he asks, clearly demonstrating that he hasn't!)
>
> Regards,
> Tim
>
>
>
Re: [classlib][luni] TreeMap woes
Posted by Tim Ellison <t....@gmail.com>.
Nathan Beyer wrote:
> It depends. Have we learned our lesson?
What lesson is that?
(he asks, clearly demonstrating that he hasn't!)
Regards,
Tim
Re: [classlib][luni] TreeMap woes
Posted by Nathan Beyer <nb...@gmail.com>.
It depends. Have we learned our lesson?
-Nathan
On Dec 8, 2007 12:04 PM, Alexey Petrenko <al...@gmail.com> wrote:
> So I think that we should keep the patch.
>
> Objections? Tim?
>
> SY, Alexey
>
> 2007/12/7, Sergey Kuksenko <se...@gmail.com>:
>
> > Hi All,
> >
> > I really found a bug. But I wish to note, that it is a bug in Eclipse.
> > The story:
> > In order to caught that I've created a "mixed" TreeMap. The mixed tree map
> > aggregates two TreeMaps - old implementation and new implementation. Also
> > mixed TreeMap performs all operation on both maps and compared results.
> > Using that I've cought the place where differentce occures.
> > It was operation remove originally from TreeSet.
> > Let see.
> > Before we have two identical maps:
> >
> > Old - { 43731146; 91780893; 92440425; }
> > New - { 43731146; 91780893; 92440425; }
> >
> > Numbers are identityHashCode values.
> > Element "91780893" comes to remove.
> > After remove we got two different maps:
> >
> > Old - { 43731146; 92440425; }
> > New - { 43731146; 91780893; 92440425; }
> >
> > If we have (before remove) elements in this order(for both threes) - it
> > means that elements has the following order 43731146 < 91780893 < 92440425
> > BUT! I found that Eclipse's comparator used here changed the behaviour.
> > According to my logs, sign(cmp(43731146,91780893)) == 1 that means that
> > 43731146 > 91780893
> >
> > If we compare "key to remove" with all element we got the follwoing signs:
> > -1,0,-1. This is a error.
> > Why RI doesn't fail at this case. It's happiness. Because of on RI 91780893
> > (key to remove) is on root and we compare key with the root first, got
> > equality and all ok. Occaisionally.
> > On my implementation we do the comparison 91780893 with 43731146(first
> > element), got the fact that "key to remove" is less then smallest element in
> > Tree and we have nothing to remove.
> >
> > So I think we shouldn't rollback the pacth, but it makes sence to find a
> > error in Eclipse and fill a bug against them.
> >
> >
> > Note, as wrote before, I found one bug in TreeMap. But, the can't impact on
> > Eclipse unit tests, because of according my logs all trees in EUT smaller
> > then 100. But the bug can took a place with small probability for trees more
> > then 64*3 elemens and can't be for trees with size smaller then 3*64.
> >
> >
> > On 12/7/07, Tim Ellison <t....@gmail.com> wrote:
> > >
> > > Sergey Kuksenko wrote:
> > > > I found a bug.
> > > > Let's wait until I am checking.
> > >
> > > Only if you promise it is the last bug ;-) Why cram it in now and
> > > suffer these new bugs and crashes?
> > >
> > > I believe the skill in making a release is knowing what to leave out,
> > > not how much you can squeeze in at the last minute.
> > >
> > > Regards,
> > > Tim
> > >
> > > > On 12/7/07, Tim Ellison <t....@gmail.com> wrote:
> > > >> This change to TreeMap seems to have introduced an instability, and I
> > > >> would say it is a candidate for reverting in M4.
> > > >>
> > > >> WDYT?
> > > >>
> > > >> ---
> > > >> Sergey Kuksenko.
> > > >> Intel Enterprise Solutions Software Division.
> > > >
> > >
> >
> >
> >
> > --
> > Best regards,
> > ---
> > Sergey Kuksenko.
> > Intel Enterprise Solutions Software Division.
> >
>
Re: [classlib][luni] TreeMap woes
Posted by Sergey Kuksenko <se...@gmail.com>.
I almost caugth the problem in Eclipse.
I'll sent details tomorrow.
On 12/10/07, Tim Ellison <t....@gmail.com> wrote:
>
> Alexey Petrenko wrote:
> > 2007/12/9, Tim Ellison <t....@gmail.com>:
> >> Alexey Petrenko wrote:
> >>> So I think that we should keep the patch.
> >> Even if it means breaking a key application?
> > Most likely it just affects the tests.
> >
> > The failing tests are testAddWorkingSet, testGetWorkingSets and
> > testRemoveWorkingSet tests in ui suite on Windows.
> >
> > So I suggest the following:
> > 1. Start our pre-milestone testing as planned (as far as I know Stepan
> > is going to start testing on Sunday morning)
> > 2. Continue investigate the failures to make sure that these incorrect
> > comparators are from unit tests but not from Eclipse itself.
> >
> > Thoughts?
>
> I agree.
>
> Regards,
> Tim
>
--
Best regards,
---
Sergey Kuksenko.
Intel Enterprise Solutions Software Division.
Re: [classlib][luni] TreeMap woes
Posted by Tim Ellison <t....@gmail.com>.
Alexey Petrenko wrote:
> 2007/12/9, Tim Ellison <t....@gmail.com>:
>> Alexey Petrenko wrote:
>>> So I think that we should keep the patch.
>> Even if it means breaking a key application?
> Most likely it just affects the tests.
>
> The failing tests are testAddWorkingSet, testGetWorkingSets and
> testRemoveWorkingSet tests in ui suite on Windows.
>
> So I suggest the following:
> 1. Start our pre-milestone testing as planned (as far as I know Stepan
> is going to start testing on Sunday morning)
> 2. Continue investigate the failures to make sure that these incorrect
> comparators are from unit tests but not from Eclipse itself.
>
> Thoughts?
I agree.
Regards,
Tim
Re: [classlib][luni] TreeMap woes
Posted by Alexey Petrenko <al...@gmail.com>.
2007/12/9, Tim Ellison <t....@gmail.com>:
> Alexey Petrenko wrote:
> > So I think that we should keep the patch.
> Even if it means breaking a key application?
Most likely it just affects the tests.
The failing tests are testAddWorkingSet, testGetWorkingSets and
testRemoveWorkingSet tests in ui suite on Windows.
So I suggest the following:
1. Start our pre-milestone testing as planned (as far as I know Stepan
is going to start testing on Sunday morning)
2. Continue investigate the failures to make sure that these incorrect
comparators are from unit tests but not from Eclipse itself.
Thoughts?
SY, Alexey
Re: [classlib][luni] TreeMap woes
Posted by Tim Ellison <t....@gmail.com>.
Alexey Petrenko wrote:
> So I think that we should keep the patch.
Even if it means breaking a key application?
Perhaps we can even check for this special case for a while.
What we really want is the best of both, i.e. performance and
compatibility. Have you looked at the other version of TreeMap that
Jimmy has modified to improve performance?
Regards,
Tim
Re: [classlib][luni] TreeMap woes
Posted by Alexey Petrenko <al...@gmail.com>.
So I think that we should keep the patch.
Objections? Tim?
SY, Alexey
2007/12/7, Sergey Kuksenko <se...@gmail.com>:
> Hi All,
>
> I really found a bug. But I wish to note, that it is a bug in Eclipse.
> The story:
> In order to caught that I've created a "mixed" TreeMap. The mixed tree map
> aggregates two TreeMaps - old implementation and new implementation. Also
> mixed TreeMap performs all operation on both maps and compared results.
> Using that I've cought the place where differentce occures.
> It was operation remove originally from TreeSet.
> Let see.
> Before we have two identical maps:
>
> Old - { 43731146; 91780893; 92440425; }
> New - { 43731146; 91780893; 92440425; }
>
> Numbers are identityHashCode values.
> Element "91780893" comes to remove.
> After remove we got two different maps:
>
> Old - { 43731146; 92440425; }
> New - { 43731146; 91780893; 92440425; }
>
> If we have (before remove) elements in this order(for both threes) - it
> means that elements has the following order 43731146 < 91780893 < 92440425
> BUT! I found that Eclipse's comparator used here changed the behaviour.
> According to my logs, sign(cmp(43731146,91780893)) == 1 that means that
> 43731146 > 91780893
>
> If we compare "key to remove" with all element we got the follwoing signs:
> -1,0,-1. This is a error.
> Why RI doesn't fail at this case. It's happiness. Because of on RI 91780893
> (key to remove) is on root and we compare key with the root first, got
> equality and all ok. Occaisionally.
> On my implementation we do the comparison 91780893 with 43731146(first
> element), got the fact that "key to remove" is less then smallest element in
> Tree and we have nothing to remove.
>
> So I think we shouldn't rollback the pacth, but it makes sence to find a
> error in Eclipse and fill a bug against them.
>
>
> Note, as wrote before, I found one bug in TreeMap. But, the can't impact on
> Eclipse unit tests, because of according my logs all trees in EUT smaller
> then 100. But the bug can took a place with small probability for trees more
> then 64*3 elemens and can't be for trees with size smaller then 3*64.
>
>
> On 12/7/07, Tim Ellison <t....@gmail.com> wrote:
> >
> > Sergey Kuksenko wrote:
> > > I found a bug.
> > > Let's wait until I am checking.
> >
> > Only if you promise it is the last bug ;-) Why cram it in now and
> > suffer these new bugs and crashes?
> >
> > I believe the skill in making a release is knowing what to leave out,
> > not how much you can squeeze in at the last minute.
> >
> > Regards,
> > Tim
> >
> > > On 12/7/07, Tim Ellison <t....@gmail.com> wrote:
> > >> This change to TreeMap seems to have introduced an instability, and I
> > >> would say it is a candidate for reverting in M4.
> > >>
> > >> WDYT?
> > >>
> > >> ---
> > >> Sergey Kuksenko.
> > >> Intel Enterprise Solutions Software Division.
> > >
> >
>
>
>
> --
> Best regards,
> ---
> Sergey Kuksenko.
> Intel Enterprise Solutions Software Division.
>
Re: [classlib][luni] TreeMap woes
Posted by Ilya Berezhniuk <il...@gmail.com>.
If this bug is not in Eclipse, but in EUT, I suggest keeping TreeMap patches.
We already have several bugs filed on EUT. Significant part of these
bugs is caused by incorrect checks/comparisons relying on Sun
behavior.
So if this bug affects only incorrect tests and does not affect key
applications, we should keep our implementation, I think.
Thanks,
Ilya.
2007/12/9, Alexey Petrenko <al...@gmail.com>:
> 2007/12/9, Tim Ellison <t....@gmail.com>:
> > Sergey Kuksenko wrote:
> > > I really found a bug. But I wish to note, that it is a bug in Eclipse.
> > > The story:
> > > In order to caught that I've created a "mixed" TreeMap. The mixed tree map
> > > aggregates two TreeMaps - old implementation and new implementation. Also
> > > mixed TreeMap performs all operation on both maps and compared results.
> > > Using that I've cought the place where differentce occures.
> > > It was operation remove originally from TreeSet.
> > > Let see.
> > > Before we have two identical maps:
> > >
> > > Old - { 43731146; 91780893; 92440425; }
> > > New - { 43731146; 91780893; 92440425; }
> > >
> > > Numbers are identityHashCode values.
> > > Element "91780893" comes to remove.
> > > After remove we got two different maps:
> > >
> > > Old - { 43731146; 92440425; }
> > > New - { 43731146; 91780893; 92440425; }
> > >
> > > If we have (before remove) elements in this order(for both threes) - it
> > > means that elements has the following order 43731146 < 91780893 < 92440425
> > > BUT! I found that Eclipse's comparator used here changed the behaviour.
> > > According to my logs, sign(cmp(43731146,91780893)) == 1 that means that
> > > 43731146 > 91780893
> > >
> > > If we compare "key to remove" with all element we got the follwoing signs:
> > > -1,0,-1. This is a error.
> >
> > I've seen a number of cases where applications do not implement to the
> > spec properly, and comparators seems to be a common case for mistakes
> > for some reason.
> >
> > > Why RI doesn't fail at this case. It's happiness. Because of on RI 91780893
> > > (key to remove) is on root and we compare key with the root first, got
> > > equality and all ok. Occaisionally.
> > > On my implementation we do the comparison 91780893 with 43731146(first
> > > element), got the fact that "key to remove" is less then smallest element in
> > > Tree and we have nothing to remove.
> >
> > This kind of behavior difference in the face of misbehaving application
> > is particularly difficult to match.
> >
> > > So I think we shouldn't rollback the pacth, but it makes sence to find a
> > > error in Eclipse and fill a bug against them.
> >
> > I definitely think it is worth filing a bug against Eclipse.
> >
> > Now what to do about the Harmony enhancement that causes Eclipse to
> > fail? Do we really want to break for all the versions of Eclipse before
> > this bug of theirs is fixed?
> I would not say that we will break Eclipse. We'll just break three
> incorrect tests from Eclipse test suite.
> And unfortunately these tests are not the only tests failing in EUT on
> Harmony...
>
> SY, Alexey
>
> >
> > > Note, as wrote before, I found one bug in TreeMap. But, the can't impact on
> > > Eclipse unit tests, because of according my logs all trees in EUT smaller
> > > then 100. But the bug can took a place with small probability for trees more
> > > then 64*3 elemens and can't be for trees with size smaller then 3*64.
> >
> > I see Alexey patched that already.
> >
> > Regards,
> > Tim
> >
>
Re: [classlib][luni] TreeMap woes
Posted by Alexey Petrenko <al...@gmail.com>.
2007/12/9, Tim Ellison <t....@gmail.com>:
> Sergey Kuksenko wrote:
> > I really found a bug. But I wish to note, that it is a bug in Eclipse.
> > The story:
> > In order to caught that I've created a "mixed" TreeMap. The mixed tree map
> > aggregates two TreeMaps - old implementation and new implementation. Also
> > mixed TreeMap performs all operation on both maps and compared results.
> > Using that I've cought the place where differentce occures.
> > It was operation remove originally from TreeSet.
> > Let see.
> > Before we have two identical maps:
> >
> > Old - { 43731146; 91780893; 92440425; }
> > New - { 43731146; 91780893; 92440425; }
> >
> > Numbers are identityHashCode values.
> > Element "91780893" comes to remove.
> > After remove we got two different maps:
> >
> > Old - { 43731146; 92440425; }
> > New - { 43731146; 91780893; 92440425; }
> >
> > If we have (before remove) elements in this order(for both threes) - it
> > means that elements has the following order 43731146 < 91780893 < 92440425
> > BUT! I found that Eclipse's comparator used here changed the behaviour.
> > According to my logs, sign(cmp(43731146,91780893)) == 1 that means that
> > 43731146 > 91780893
> >
> > If we compare "key to remove" with all element we got the follwoing signs:
> > -1,0,-1. This is a error.
>
> I've seen a number of cases where applications do not implement to the
> spec properly, and comparators seems to be a common case for mistakes
> for some reason.
>
> > Why RI doesn't fail at this case. It's happiness. Because of on RI 91780893
> > (key to remove) is on root and we compare key with the root first, got
> > equality and all ok. Occaisionally.
> > On my implementation we do the comparison 91780893 with 43731146(first
> > element), got the fact that "key to remove" is less then smallest element in
> > Tree and we have nothing to remove.
>
> This kind of behavior difference in the face of misbehaving application
> is particularly difficult to match.
>
> > So I think we shouldn't rollback the pacth, but it makes sence to find a
> > error in Eclipse and fill a bug against them.
>
> I definitely think it is worth filing a bug against Eclipse.
>
> Now what to do about the Harmony enhancement that causes Eclipse to
> fail? Do we really want to break for all the versions of Eclipse before
> this bug of theirs is fixed?
I would not say that we will break Eclipse. We'll just break three
incorrect tests from Eclipse test suite.
And unfortunately these tests are not the only tests failing in EUT on
Harmony...
SY, Alexey
>
> > Note, as wrote before, I found one bug in TreeMap. But, the can't impact on
> > Eclipse unit tests, because of according my logs all trees in EUT smaller
> > then 100. But the bug can took a place with small probability for trees more
> > then 64*3 elemens and can't be for trees with size smaller then 3*64.
>
> I see Alexey patched that already.
>
> Regards,
> Tim
>
Re: [classlib][luni] TreeMap woes
Posted by Tim Ellison <t....@gmail.com>.
Sergey Kuksenko wrote:
> I really found a bug. But I wish to note, that it is a bug in Eclipse.
> The story:
> In order to caught that I've created a "mixed" TreeMap. The mixed tree map
> aggregates two TreeMaps - old implementation and new implementation. Also
> mixed TreeMap performs all operation on both maps and compared results.
> Using that I've cought the place where differentce occures.
> It was operation remove originally from TreeSet.
> Let see.
> Before we have two identical maps:
>
> Old - { 43731146; 91780893; 92440425; }
> New - { 43731146; 91780893; 92440425; }
>
> Numbers are identityHashCode values.
> Element "91780893" comes to remove.
> After remove we got two different maps:
>
> Old - { 43731146; 92440425; }
> New - { 43731146; 91780893; 92440425; }
>
> If we have (before remove) elements in this order(for both threes) - it
> means that elements has the following order 43731146 < 91780893 < 92440425
> BUT! I found that Eclipse's comparator used here changed the behaviour.
> According to my logs, sign(cmp(43731146,91780893)) == 1 that means that
> 43731146 > 91780893
>
> If we compare "key to remove" with all element we got the follwoing signs:
> -1,0,-1. This is a error.
I've seen a number of cases where applications do not implement to the
spec properly, and comparators seems to be a common case for mistakes
for some reason.
> Why RI doesn't fail at this case. It's happiness. Because of on RI 91780893
> (key to remove) is on root and we compare key with the root first, got
> equality and all ok. Occaisionally.
> On my implementation we do the comparison 91780893 with 43731146(first
> element), got the fact that "key to remove" is less then smallest element in
> Tree and we have nothing to remove.
This kind of behavior difference in the face of misbehaving application
is particularly difficult to match.
> So I think we shouldn't rollback the pacth, but it makes sence to find a
> error in Eclipse and fill a bug against them.
I definitely think it is worth filing a bug against Eclipse.
Now what to do about the Harmony enhancement that causes Eclipse to
fail? Do we really want to break for all the versions of Eclipse before
this bug of theirs is fixed?
> Note, as wrote before, I found one bug in TreeMap. But, the can't impact on
> Eclipse unit tests, because of according my logs all trees in EUT smaller
> then 100. But the bug can took a place with small probability for trees more
> then 64*3 elemens and can't be for trees with size smaller then 3*64.
I see Alexey patched that already.
Regards,
Tim
Re: [classlib][luni] TreeMap woes
Posted by Sergey Kuksenko <se...@gmail.com>.
Hi All,
I really found a bug. But I wish to note, that it is a bug in Eclipse.
The story:
In order to caught that I've created a "mixed" TreeMap. The mixed tree map
aggregates two TreeMaps - old implementation and new implementation. Also
mixed TreeMap performs all operation on both maps and compared results.
Using that I've cought the place where differentce occures.
It was operation remove originally from TreeSet.
Let see.
Before we have two identical maps:
Old - { 43731146; 91780893; 92440425; }
New - { 43731146; 91780893; 92440425; }
Numbers are identityHashCode values.
Element "91780893" comes to remove.
After remove we got two different maps:
Old - { 43731146; 92440425; }
New - { 43731146; 91780893; 92440425; }
If we have (before remove) elements in this order(for both threes) - it
means that elements has the following order 43731146 < 91780893 < 92440425
BUT! I found that Eclipse's comparator used here changed the behaviour.
According to my logs, sign(cmp(43731146,91780893)) == 1 that means that
43731146 > 91780893
If we compare "key to remove" with all element we got the follwoing signs:
-1,0,-1. This is a error.
Why RI doesn't fail at this case. It's happiness. Because of on RI 91780893
(key to remove) is on root and we compare key with the root first, got
equality and all ok. Occaisionally.
On my implementation we do the comparison 91780893 with 43731146(first
element), got the fact that "key to remove" is less then smallest element in
Tree and we have nothing to remove.
So I think we shouldn't rollback the pacth, but it makes sence to find a
error in Eclipse and fill a bug against them.
Note, as wrote before, I found one bug in TreeMap. But, the can't impact on
Eclipse unit tests, because of according my logs all trees in EUT smaller
then 100. But the bug can took a place with small probability for trees more
then 64*3 elemens and can't be for trees with size smaller then 3*64.
On 12/7/07, Tim Ellison <t....@gmail.com> wrote:
>
> Sergey Kuksenko wrote:
> > I found a bug.
> > Let's wait until I am checking.
>
> Only if you promise it is the last bug ;-) Why cram it in now and
> suffer these new bugs and crashes?
>
> I believe the skill in making a release is knowing what to leave out,
> not how much you can squeeze in at the last minute.
>
> Regards,
> Tim
>
> > On 12/7/07, Tim Ellison <t....@gmail.com> wrote:
> >> This change to TreeMap seems to have introduced an instability, and I
> >> would say it is a candidate for reverting in M4.
> >>
> >> WDYT?
> >>
> >> ---
> >> Sergey Kuksenko.
> >> Intel Enterprise Solutions Software Division.
> >
>
--
Best regards,
---
Sergey Kuksenko.
Intel Enterprise Solutions Software Division.
Re: [classlib][luni] TreeMap woes
Posted by Tim Ellison <t....@gmail.com>.
Sergey Kuksenko wrote:
> I found a bug.
> Let's wait until I am checking.
Only if you promise it is the last bug ;-) Why cram it in now and
suffer these new bugs and crashes?
I believe the skill in making a release is knowing what to leave out,
not how much you can squeeze in at the last minute.
Regards,
Tim
> On 12/7/07, Tim Ellison <t....@gmail.com> wrote:
>> This change to TreeMap seems to have introduced an instability, and I
>> would say it is a candidate for reverting in M4.
>>
>> WDYT?
>>
>> ---
>> Sergey Kuksenko.
>> Intel Enterprise Solutions Software Division.
>
Re: [classlib][luni] TreeMap woes (was: Re: [jira] Commented: (HARMONY-5265) [classlib][luni] TreeMap bug fixing and further performance improvements)
Posted by Sergey Kuksenko <se...@gmail.com>.
I found a bug.
Let's wait until I am checking.
On 12/7/07, Tim Ellison <t....@gmail.com> wrote:
>
> This change to TreeMap seems to have introduced an instability, and I
> would say it is a candidate for reverting in M4.
>
> WDYT?
>
> ---
> Sergey Kuksenko.
> Intel Enterprise Solutions Software Division.