harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nathan Beyer <nbe...@gmail.com>
Subject Re: [jira] Created: (HARMONY-6404) possible data-reordering in some hashCode-methods (e.g. String or URL)
Date Mon, 21 Dec 2009 05:30:15 GMT
On Sun, Dec 20, 2009 at 1:42 PM, Tim Ellison <t.p.ellison@gmail.com> wrote:
> On 19/Dec/2009 20:54, Nathan Beyer wrote:
>> On Sat, Dec 19, 2009 at 2:26 PM, Tim Ellison <t.p.ellison@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 <egor.pasko@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
>>>>>
>>>>>
>>
>

Mime
View raw message