You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by Egor Pasko <eg...@gmail.com> on 2009/12/12 11:54:25 UTC
Re: [jira] Created: (HARMONY-6404) possible data-reordering in some hashCode-methods (e.g. String or URL)
On the 0x685 day of Apache Harmony Nathan Beyer wrote:
> On Fri, Dec 11, 2009 at 10:04 AM, Tim Ellison <t....@gmail.com> wrote:
>> On 11/Dec/2009 14:32, Egor Pasko wrote:
>>> On the 0x684 day of Apache Harmony Tim Ellison wrote:
>>>> On 11/Dec/2009 04:09, Vijay Menon wrote:
>>>>> Perhaps I'm missing some context, but I don't see any problem. I don't
>>>>> believe that this:
>>>>>
>>>>> if (hashCode == 0) {
>>>>> // calculate hash
>>>>> hashCode = hash;
>>>>> }
>>>>> return hashCode;
>>>>>
>>>>> can ever return 0 (assuming hash is non-zero), regardless of memory fences.
>>>>> The JMM won't allow visible reordering in a single threaded program.
>>>> I agree. In the multi-threaded case there can be a data race on the
>>>> hashCode, with the effect that the same hashCode value is unnecessarily,
>>>> but harmlessly, recalculated.
>>>
>>> Vijay, Tim, you are not 100% correct here.
>>>
>>> 1. there should be an extra restriction that the part "calculate hash"
>>> does not touch the hashCode field. Without that restriction more
>>> trivial races can happen as discussed in LANG-481.
>>>
>>> So we should assume this code:
>>>
>>> if (this.hashCode == 0) {
>>> int hash;
>>> if (this.hashCode == 0) {
>>> // Calculate using 'hash' only, not this.hashCode.
>>> this.hashCode = hash;
>>> }
>>> return this.hashCode;
>>> }
>>
>> Yes, I guess I figured that was assumed :-)
>>
>> Of course, there are lots of things you could put into the
>> "// Calculate..." section that would be unsafe. We should stick with
>> showing the non-abbreviated implementation to avoid ambiguity:
>>
>> public int hashCode() {
>> if (hashCode == 0) {
>> if (count == 0) {
>> return 0;
>> }
>> int hash = 0;
>> for (int i = offset; i < count + offset; i++) {
>> hash = value[i] + ((hash << 5) - hash);
>> }
>> hashCode = hash;
>> }
>> return hashCode;
>> }
>>
>
> I think I understand the concern, after some additional reading. The
> issue seems to be that 'hashCode' is read twice and the field is not
> protected by any memory barriers (synchronized, volatile, etc). As
> such, it would be okay for the second read to be done using a cached
> value, which means that both reads could return 0 in the same thread
> of execution. Another way to look at it is that the write to
> 'hashCode' may or may not affect subsequent reads of 'hashCode'. This
> is how I understand it.
>
> Will that happen in practice? I have no idea. It does seem possible.
>
> In any case, it does seem a pinch more efficient to only do one read
> of hashCode ... switch up the code to be something like this.
>
> public int hashCode() {
> final int hash = hashCode;
> if (hash == 0) {
> if (count == 0) {
> return 0;
> }
> for (int i = offset; i < count + offset; i++) {
> hash = value[i] + ((hash << 5) - hash);
> }
> hashCode = hash;
one more 'return hash' here, please :)
> }
> return hash;
> }
>
> Thoughts?
--
Egor Pasko
Re: [jira] Created: (HARMONY-6404) possible data-reordering in some
hashCode-methods (e.g. String or URL)
Posted by Vijay Menon <vs...@google.com>.
2010/1/9 Egor Pasko <eg...@gmail.com>
> On the 0x68E day of Apache Harmony Nathan Beyer wrote:
> > On Sun, Dec 20, 2009 at 1:42 PM, Tim Ellison <t....@gmail.com>
> wrote:
> >> On 19/Dec/2009 20:54, Nathan Beyer wrote:
> >>> On Sat, Dec 19, 2009 at 2:26 PM, Tim Ellison <t....@gmail.com>
> wrote:
> >>>> On 19/Dec/2009 18:57, Nathan Beyer wrote:
> >>>>> RE: https://issues.apache.org/jira/browse/HARMONY-6404
> >>>>>
> >>>>> I posted up two proposed patches for String, please comment on which
> >>>>> is preferable. One is the change previously mentioned, the other is a
> >>>>> slightly bigger reorganization.
> >>>> Neither. There is nothing wrong with the original code.
> >>>>
> >>>> Any JVM or compiler that would actually move a load up above a
> >>>> conditional store to the same variable would be seriously broken all
> round.
> >>>
> >>> Does that really matter?
> >>
> >> Yes, we don't code to allow for bogus VM/compiler implementations.
> >>
> >>> Shouldn't the Java source be written such that it's as correct as
> >>> possible according to the language spec and related specs?
> >>
> >> I'm guessing you have your tongue firmly in your cheek when writing that
> >> :-) Yes to "correct as possible" but we don't account for the never to
> >> be implemented edge cases that are possible by the spec.
> >>
> >> Put another way, we don't fix things that are not broken. I challenge
> >> you to show an implementation that would do the read re-ordering as you
> >> suggest.
> >
> > So what's all the fuss, then? According to this guy [1] ... our code
> > is broken and he helped define the memory model. This is the guy
> > suggesting the issue. Our code is almost exactly like the same he
> > describes as broken. The only difference is we have an extra check on
> > 'count'.
> >
> > At this point, I just want to understand.
> >
> > [1]
> http://jeremymanson.blogspot.com/2008/12/benign-data-races-in-java.html
>
> Sorry for delayed answer (was on vacation).
>
> After all the fuss I think Tim is right. JMM is a model that is close
> to real implementations except some corner cases like this. JMM allows
> slightly more than any JVM implementation would do. That's a good
> excersise to think about and be warned, but not a lot more.
>
> If our goal is to please all possible JVM implementations, we should
> bother, but that would be far from the main goal of the project. Until
> we have a meaningful implementation to make two memory loads in the
> place where only one is required, we should not take any action on
> hashCode.
>
>
I hate to belabor the point, but the code is legally broken and Nathan's
proposed fix is simple and straightforward. Why not fix it?
I agree that it's unlikely to break on x86. But, I think it can break on
practical implementations for other architectures. From memory, StarJIT's
Itanium backend would do exactly the reordering Jeremy Manson mentions in
his blog post under certain profile information and scheduling constraints.
Vijay
> --
> Egor Pasko
>
>
Re: [jira] Created: (HARMONY-6404) possible data-reordering in some
hashCode-methods (e.g. String or URL)
Posted by Nathan Beyer <nd...@apache.org>.
2010/1/9 Egor Pasko <eg...@gmail.com>:
> On the 0x68E day of Apache Harmony Nathan Beyer wrote:
>> On Sun, Dec 20, 2009 at 1:42 PM, Tim Ellison <t....@gmail.com> wrote:
>>> On 19/Dec/2009 20:54, Nathan Beyer wrote:
>>>> On Sat, Dec 19, 2009 at 2:26 PM, Tim Ellison <t....@gmail.com> wrote:
>>>>> On 19/Dec/2009 18:57, Nathan Beyer wrote:
>>>>>> RE: https://issues.apache.org/jira/browse/HARMONY-6404
>>>>>>
>>>>>> I posted up two proposed patches for String, please comment on which
>>>>>> is preferable. One is the change previously mentioned, the other is a
>>>>>> slightly bigger reorganization.
>>>>> Neither. There is nothing wrong with the original code.
>>>>>
>>>>> Any JVM or compiler that would actually move a load up above a
>>>>> conditional store to the same variable would be seriously broken all round.
>>>>
>>>> Does that really matter?
>>>
>>> Yes, we don't code to allow for bogus VM/compiler implementations.
>>>
>>>> Shouldn't the Java source be written such that it's as correct as
>>>> possible according to the language spec and related specs?
>>>
>>> I'm guessing you have your tongue firmly in your cheek when writing that
>>> :-) Yes to "correct as possible" but we don't account for the never to
>>> be implemented edge cases that are possible by the spec.
>>>
>>> Put another way, we don't fix things that are not broken. I challenge
>>> you to show an implementation that would do the read re-ordering as you
>>> suggest.
>>
>> So what's all the fuss, then? According to this guy [1] ... our code
>> is broken and he helped define the memory model. This is the guy
>> suggesting the issue. Our code is almost exactly like the same he
>> describes as broken. The only difference is we have an extra check on
>> 'count'.
>>
>> At this point, I just want to understand.
>>
>> [1] http://jeremymanson.blogspot.com/2008/12/benign-data-races-in-java.html
>
> Sorry for delayed answer (was on vacation).
>
> After all the fuss I think Tim is right. JMM is a model that is close
> to real implementations except some corner cases like this. JMM allows
> slightly more than any JVM implementation would do. That's a good
> excersise to think about and be warned, but not a lot more.
>
> If our goal is to please all possible JVM implementations, we should
> bother, but that would be far from the main goal of the project. Until
> we have a meaningful implementation to make two memory loads in the
> place where only one is required, we should not take any action on
> hashCode.
>
> --
> Egor Pasko
Since this issue doesn't have clear consensus, at least not to me, I'd
like to get an explicit vote for resolving this issue as "Wont Fix".
https://issues.apache.org/jira/browse/HARMONY-6404
+1 to close as "Wont Fix"
-1 to leave open - please accompany with a patch
Re: [jira] Created: (HARMONY-6404) possible data-reordering in some hashCode-methods (e.g. String or URL)
Posted by Egor Pasko <eg...@gmail.com>.
On the 0x68E day of Apache Harmony Nathan Beyer wrote:
> On Sun, Dec 20, 2009 at 1:42 PM, Tim Ellison <t....@gmail.com> wrote:
>> On 19/Dec/2009 20:54, Nathan Beyer wrote:
>>> On Sat, Dec 19, 2009 at 2:26 PM, Tim Ellison <t....@gmail.com> wrote:
>>>> On 19/Dec/2009 18:57, Nathan Beyer wrote:
>>>>> RE: https://issues.apache.org/jira/browse/HARMONY-6404
>>>>>
>>>>> I posted up two proposed patches for String, please comment on which
>>>>> is preferable. One is the change previously mentioned, the other is a
>>>>> slightly bigger reorganization.
>>>> Neither. There is nothing wrong with the original code.
>>>>
>>>> Any JVM or compiler that would actually move a load up above a
>>>> conditional store to the same variable would be seriously broken all round.
>>>
>>> Does that really matter?
>>
>> Yes, we don't code to allow for bogus VM/compiler implementations.
>>
>>> Shouldn't the Java source be written such that it's as correct as
>>> possible according to the language spec and related specs?
>>
>> I'm guessing you have your tongue firmly in your cheek when writing that
>> :-) Yes to "correct as possible" but we don't account for the never to
>> be implemented edge cases that are possible by the spec.
>>
>> Put another way, we don't fix things that are not broken. I challenge
>> you to show an implementation that would do the read re-ordering as you
>> suggest.
>
> So what's all the fuss, then? According to this guy [1] ... our code
> is broken and he helped define the memory model. This is the guy
> suggesting the issue. Our code is almost exactly like the same he
> describes as broken. The only difference is we have an extra check on
> 'count'.
>
> At this point, I just want to understand.
>
> [1] http://jeremymanson.blogspot.com/2008/12/benign-data-races-in-java.html
Sorry for delayed answer (was on vacation).
After all the fuss I think Tim is right. JMM is a model that is close
to real implementations except some corner cases like this. JMM allows
slightly more than any JVM implementation would do. That's a good
excersise to think about and be warned, but not a lot more.
If our goal is to please all possible JVM implementations, we should
bother, but that would be far from the main goal of the project. Until
we have a meaningful implementation to make two memory loads in the
place where only one is required, we should not take any action on
hashCode.
--
Egor Pasko
Re: [jira] Created: (HARMONY-6404) possible data-reordering in some
hashCode-methods (e.g. String or URL)
Posted by Nathan Beyer <nb...@gmail.com>.
On Sun, Dec 20, 2009 at 1:42 PM, Tim Ellison <t....@gmail.com> wrote:
> On 19/Dec/2009 20:54, Nathan Beyer wrote:
>> On Sat, Dec 19, 2009 at 2:26 PM, Tim Ellison <t....@gmail.com> wrote:
>>> On 19/Dec/2009 18:57, Nathan Beyer wrote:
>>>> RE: https://issues.apache.org/jira/browse/HARMONY-6404
>>>>
>>>> I posted up two proposed patches for String, please comment on which
>>>> is preferable. One is the change previously mentioned, the other is a
>>>> slightly bigger reorganization.
>>> Neither. There is nothing wrong with the original code.
>>>
>>> Any JVM or compiler that would actually move a load up above a
>>> conditional store to the same variable would be seriously broken all round.
>>
>> Does that really matter?
>
> Yes, we don't code to allow for bogus VM/compiler implementations.
>
>> Shouldn't the Java source be written such that it's as correct as
>> possible according to the language spec and related specs?
>
> I'm guessing you have your tongue firmly in your cheek when writing that
> :-) Yes to "correct as possible" but we don't account for the never to
> be implemented edge cases that are possible by the spec.
>
> Put another way, we don't fix things that are not broken. I challenge
> you to show an implementation that would do the read re-ordering as you
> suggest.
So what's all the fuss, then? According to this guy [1] ... our code
is broken and he helped define the memory model. This is the guy
suggesting the issue. Our code is almost exactly like the same he
describes as broken. The only difference is we have an extra check on
'count'.
At this point, I just want to understand.
[1] http://jeremymanson.blogspot.com/2008/12/benign-data-races-in-java.html
>
> In any case, if you want to make the changes, go ahead, I don't think it
> will break/fix anything.
>
>> Regardless, this isn't about ordering, its about 'this.hashCode' being
>> read multiple times -- which is the exact type of change that was just
>> fixed in the equals method [1]. How are these two methods different?
>> Why change one and not the other.
>
> They are different because the multiple loads by the same thread make a
> difference in the equals() method. If the hashCode changes between
> loads by the same thread then the equals would answer the wrong value.
>
> However, the hashCode method is different because a thread can't return
> the wrong value by reading the hashCode twice. There may be a data
> race, with two threads both calculating the hash, but they will simply
> compute the same value and store it, so no harm done.
>
> Regards,
> Tim
>
>> [1] https://issues.apache.org/jira/browse/HARMONY-6405
>>
>>> Regards,
>>> Tim
>>>
>>>
>>>> On Sat, Dec 19, 2009 at 4:04 AM, Egor Pasko <eg...@gmail.com> wrote:
>>>>> On the 0x68A day of Apache Harmony Tim Ellison wrote:
>>>>>> On 12/Dec/2009 10:54, Egor Pasko wrote:
>>>>>>> On the 0x685 day of Apache Harmony Nathan Beyer wrote:
>>>>>> <snip>
>>>>>>>> In any case, it does seem a pinch more efficient to only do one read
>>>>>>>> of hashCode ... switch up the code to be something like this.
>>>>>>>>
>>>>>>>> public int hashCode() {
>>>>>>>> final int hash = hashCode;
>>>>>>>> if (hash == 0) {
>>>>>>>> if (count == 0) {
>>>>>>>> return 0;
>>>>>>>> }
>>>>>>>> for (int i = offset; i < count + offset; i++) {
>>>>>>>> hash = value[i] + ((hash << 5) - hash);
>>>>>>>> }
>>>>>>>> hashCode = hash;
>>>>>>> one more 'return hash' here, please :)
>>>>>> Why?
>>>>> argh, what was I thinking about? the original Nathan's code is great.
>>>>>
>>>>> --
>>>>> Egor Pasko
>>>>>
>>>>>
>>
>
Re: [jira] Created: (HARMONY-6404) possible data-reordering in some
hashCode-methods (e.g. String or URL)
Posted by Tim Ellison <t....@gmail.com>.
On 19/Dec/2009 20:54, Nathan Beyer wrote:
> On Sat, Dec 19, 2009 at 2:26 PM, Tim Ellison <t....@gmail.com> wrote:
>> On 19/Dec/2009 18:57, Nathan Beyer wrote:
>>> RE: https://issues.apache.org/jira/browse/HARMONY-6404
>>>
>>> I posted up two proposed patches for String, please comment on which
>>> is preferable. One is the change previously mentioned, the other is a
>>> slightly bigger reorganization.
>> Neither. There is nothing wrong with the original code.
>>
>> Any JVM or compiler that would actually move a load up above a
>> conditional store to the same variable would be seriously broken all round.
>
> Does that really matter?
Yes, we don't code to allow for bogus VM/compiler implementations.
> Shouldn't the Java source be written such that it's as correct as
> possible according to the language spec and related specs?
I'm guessing you have your tongue firmly in your cheek when writing that
:-) Yes to "correct as possible" but we don't account for the never to
be implemented edge cases that are possible by the spec.
Put another way, we don't fix things that are not broken. I challenge
you to show an implementation that would do the read re-ordering as you
suggest.
In any case, if you want to make the changes, go ahead, I don't think it
will break/fix anything.
> Regardless, this isn't about ordering, its about 'this.hashCode' being
> read multiple times -- which is the exact type of change that was just
> fixed in the equals method [1]. How are these two methods different?
> Why change one and not the other.
They are different because the multiple loads by the same thread make a
difference in the equals() method. If the hashCode changes between
loads by the same thread then the equals would answer the wrong value.
However, the hashCode method is different because a thread can't return
the wrong value by reading the hashCode twice. There may be a data
race, with two threads both calculating the hash, but they will simply
compute the same value and store it, so no harm done.
Regards,
Tim
> [1] https://issues.apache.org/jira/browse/HARMONY-6405
>
>> Regards,
>> Tim
>>
>>
>>> On Sat, Dec 19, 2009 at 4:04 AM, Egor Pasko <eg...@gmail.com> wrote:
>>>> On the 0x68A day of Apache Harmony Tim Ellison wrote:
>>>>> On 12/Dec/2009 10:54, Egor Pasko wrote:
>>>>>> On the 0x685 day of Apache Harmony Nathan Beyer wrote:
>>>>> <snip>
>>>>>>> In any case, it does seem a pinch more efficient to only do one read
>>>>>>> of hashCode ... switch up the code to be something like this.
>>>>>>>
>>>>>>> public int hashCode() {
>>>>>>> final int hash = hashCode;
>>>>>>> if (hash == 0) {
>>>>>>> if (count == 0) {
>>>>>>> return 0;
>>>>>>> }
>>>>>>> for (int i = offset; i < count + offset; i++) {
>>>>>>> hash = value[i] + ((hash << 5) - hash);
>>>>>>> }
>>>>>>> hashCode = hash;
>>>>>> one more 'return hash' here, please :)
>>>>> Why?
>>>> argh, what was I thinking about? the original Nathan's code is great.
>>>>
>>>> --
>>>> Egor Pasko
>>>>
>>>>
>
Re: [jira] Created: (HARMONY-6404) possible data-reordering in some
hashCode-methods (e.g. String or URL)
Posted by Nathan Beyer <nd...@apache.org>.
On Sat, Dec 19, 2009 at 2:26 PM, Tim Ellison <t....@gmail.com> wrote:
> On 19/Dec/2009 18:57, Nathan Beyer wrote:
>> RE: https://issues.apache.org/jira/browse/HARMONY-6404
>>
>> I posted up two proposed patches for String, please comment on which
>> is preferable. One is the change previously mentioned, the other is a
>> slightly bigger reorganization.
>
> Neither. There is nothing wrong with the original code.
>
> Any JVM or compiler that would actually move a load up above a
> conditional store to the same variable would be seriously broken all round.
Does that really matter? Shouldn't the Java source be written such
that it's as correct as possible according to the language spec and
related specs?
Regardless, this isn't about ordering, its about 'this.hashCode' being
read multiple times -- which is the exact type of change that was just
fixed in the equals method [1]. How are these two methods different?
Why change one and not the other.
-Nathan
[1] https://issues.apache.org/jira/browse/HARMONY-6405
>
> Regards,
> Tim
>
>
>> On Sat, Dec 19, 2009 at 4:04 AM, Egor Pasko <eg...@gmail.com> wrote:
>>> On the 0x68A day of Apache Harmony Tim Ellison wrote:
>>>> On 12/Dec/2009 10:54, Egor Pasko wrote:
>>>>> On the 0x685 day of Apache Harmony Nathan Beyer wrote:
>>>> <snip>
>>>>>> In any case, it does seem a pinch more efficient to only do one read
>>>>>> of hashCode ... switch up the code to be something like this.
>>>>>>
>>>>>> public int hashCode() {
>>>>>> final int hash = hashCode;
>>>>>> if (hash == 0) {
>>>>>> if (count == 0) {
>>>>>> return 0;
>>>>>> }
>>>>>> for (int i = offset; i < count + offset; i++) {
>>>>>> hash = value[i] + ((hash << 5) - hash);
>>>>>> }
>>>>>> hashCode = hash;
>>>>> one more 'return hash' here, please :)
>>>> Why?
>>> argh, what was I thinking about? the original Nathan's code is great.
>>>
>>> --
>>> Egor Pasko
>>>
>>>
>>
>
Re: [jira] Created: (HARMONY-6404) possible data-reordering in some
hashCode-methods (e.g. String or URL)
Posted by Tim Ellison <t....@gmail.com>.
On 19/Dec/2009 18:57, Nathan Beyer wrote:
> RE: https://issues.apache.org/jira/browse/HARMONY-6404
>
> I posted up two proposed patches for String, please comment on which
> is preferable. One is the change previously mentioned, the other is a
> slightly bigger reorganization.
Neither. There is nothing wrong with the original code.
Any JVM or compiler that would actually move a load up above a
conditional store to the same variable would be seriously broken all round.
Regards,
Tim
> On Sat, Dec 19, 2009 at 4:04 AM, Egor Pasko <eg...@gmail.com> wrote:
>> On the 0x68A day of Apache Harmony Tim Ellison wrote:
>>> On 12/Dec/2009 10:54, Egor Pasko wrote:
>>>> On the 0x685 day of Apache Harmony Nathan Beyer wrote:
>>> <snip>
>>>>> In any case, it does seem a pinch more efficient to only do one read
>>>>> of hashCode ... switch up the code to be something like this.
>>>>>
>>>>> public int hashCode() {
>>>>> final int hash = hashCode;
>>>>> if (hash == 0) {
>>>>> if (count == 0) {
>>>>> return 0;
>>>>> }
>>>>> for (int i = offset; i < count + offset; i++) {
>>>>> hash = value[i] + ((hash << 5) - hash);
>>>>> }
>>>>> hashCode = hash;
>>>> one more 'return hash' here, please :)
>>> Why?
>> argh, what was I thinking about? the original Nathan's code is great.
>>
>> --
>> Egor Pasko
>>
>>
>
Re: [jira] Created: (HARMONY-6404) possible data-reordering in some
hashCode-methods (e.g. String or URL)
Posted by Nathan Beyer <nd...@apache.org>.
RE: https://issues.apache.org/jira/browse/HARMONY-6404
I posted up two proposed patches for String, please comment on which
is preferable. One is the change previously mentioned, the other is a
slightly bigger reorganization.
-Nathan
On Sat, Dec 19, 2009 at 4:04 AM, Egor Pasko <eg...@gmail.com> wrote:
> On the 0x68A day of Apache Harmony Tim Ellison wrote:
>> On 12/Dec/2009 10:54, Egor Pasko wrote:
>>> On the 0x685 day of Apache Harmony Nathan Beyer wrote:
>> <snip>
>>>> In any case, it does seem a pinch more efficient to only do one read
>>>> of hashCode ... switch up the code to be something like this.
>>>>
>>>> public int hashCode() {
>>>> final int hash = hashCode;
>>>> if (hash == 0) {
>>>> if (count == 0) {
>>>> return 0;
>>>> }
>>>> for (int i = offset; i < count + offset; i++) {
>>>> hash = value[i] + ((hash << 5) - hash);
>>>> }
>>>> hashCode = hash;
>>>
>>> one more 'return hash' here, please :)
>>
>> Why?
>
> argh, what was I thinking about? the original Nathan's code is great.
>
> --
> Egor Pasko
>
>
Re: [jira] Created: (HARMONY-6404) possible data-reordering in some hashCode-methods (e.g. String or URL)
Posted by Egor Pasko <eg...@gmail.com>.
On the 0x68A day of Apache Harmony Tim Ellison wrote:
> On 12/Dec/2009 10:54, Egor Pasko wrote:
>> On the 0x685 day of Apache Harmony Nathan Beyer wrote:
> <snip>
>>> In any case, it does seem a pinch more efficient to only do one read
>>> of hashCode ... switch up the code to be something like this.
>>>
>>> public int hashCode() {
>>> final int hash = hashCode;
>>> if (hash == 0) {
>>> if (count == 0) {
>>> return 0;
>>> }
>>> for (int i = offset; i < count + offset; i++) {
>>> hash = value[i] + ((hash << 5) - hash);
>>> }
>>> hashCode = hash;
>>
>> one more 'return hash' here, please :)
>
> Why?
argh, what was I thinking about? the original Nathan's code is great.
--
Egor Pasko
Re: [jira] Created: (HARMONY-6404) possible data-reordering in some
hashCode-methods (e.g. String or URL)
Posted by Tim Ellison <t....@gmail.com>.
On 12/Dec/2009 10:54, Egor Pasko wrote:
> On the 0x685 day of Apache Harmony Nathan Beyer wrote:
<snip>
>> In any case, it does seem a pinch more efficient to only do one read
>> of hashCode ... switch up the code to be something like this.
>>
>> public int hashCode() {
>> final int hash = hashCode;
>> if (hash == 0) {
>> if (count == 0) {
>> return 0;
>> }
>> for (int i = offset; i < count + offset; i++) {
>> hash = value[i] + ((hash << 5) - hash);
>> }
>> hashCode = hash;
>
> one more 'return hash' here, please :)
Why?
>> }
>> return hash;
>> }