On 2019-04-09 15:03, Andrew Dinn wrote:
How about this:
http://cr.openjdk.java.net/~redestad/8221836/open.03/
Yes, that looks fine.
Thanks, Andrew. I'll push this shortly.
/Claes
On 09/04/2019 13:21, Claes Redestad wrote:
> On 2019-04-09 11:05, Andrew Dinn wrote:
>> It would probably also be good if you extended the comment to document
>> the status quo i.e. as Peter noted that the assigned values are computed
>> deterministically from immutable state.
>
> How about this:
On 2019-04-09 11:05, Andrew Dinn wrote:
It would probably also be good if you extended the comment to document
the status quo i.e. as Peter noted that the assigned values are computed
deterministically from immutable state.
How about this:
http://cr.openjdk.java.net/~redestad/8221836/open.03
On 4/9/19 10:53 AM, Peter Levart wrote:
> On 4/9/19 10:11 AM, Aleksey Shipilev wrote:
>>> 2. No risk of hashcode recomputation for the 2^-32 case.
>>> This might seem laughable, until you remember that it's exactly
>>> those cases that DOS attackers like to create.
>> Alt-hashing covers this obscur
On 09/04/2019 09:42, Claes Redestad wrote:
> Hi Andrew,
>
> On 2019-04-09 10:20, Andrew Dinn wrote:
>> If the patch is to go in -- and I concede there is an acceptable
>> argument for that -- then it
>> needs commenting to help avoid this happening.
>
> open.02 already adds what I believed to
On 4/9/19 10:42 AM, Claes Redestad wrote:
Hi Andrew,
On 2019-04-09 10:20, Andrew Dinn wrote:
If the patch is to go in -- and I concede there is an acceptable
argument for that -- then it
needs commenting to help avoid this happening.
open.02 already adds what I believed to be sufficient c
Hi Aleksey,
On 4/9/19 10:11 AM, Aleksey Shipilev wrote:
2. No risk of hashcode recomputation for the 2^-32 case.
This might seem laughable, until you remember that it's exactly
those cases that DOS attackers like to create.
Alt-hashing covers this obscure case in the course of mitigating much
Hi Andrew,
On 2019-04-09 10:20, Andrew Dinn wrote:
If the patch is to go in -- and I concede there is an acceptable argument for
that -- then it
needs commenting to help avoid this happening.
open.02 already adds what I believed to be sufficient commenting, have
you taken this into considerat
On 09/04/2019 09:11, Aleksey Shipilev wrote:
> On 4/8/19 11:31 PM, John Rose wrote:
>> I agree that this is a good change, and you can use me as a reviewer.
>
> Which opens up the process question: are you acting as Project Lead here to
> resolve the disagreement
> between Reviewers (the only
On 08/04/2019 22:19, Claes Redestad wrote:
> First, I disagree strongly that this patch adds significant complexity
> (especially after recent simplifications[1]) or that it risks increasing
> maintenance headache down the line.
It all depends what you mean by significant. It definitely adds
compl
On 4/8/19 11:31 PM, John Rose wrote:
> I agree that this is a good change, and you can use me as a reviewer.
Which opens up the process question: are you acting as Project Lead here to
resolve the disagreement
between Reviewers (the only accepting Reviewer being yourself)?
(There are ways for me
I agree that this is a good change, and you can use me as a reviewer.
I disagree with Aleksey; it's a new technique but not complex
to document or understand. The two state components are
independent in their action; there is no race between their
state changes.
Meanwhile, there are two reasons
On 2019-04-08 17:10, Andrew Dinn wrote:
On 08/04/2019 15:55, Aleksey Shipilev wrote:
Why introduce this complexity to begin with? That's my argument. The
complication of this code gives
us almost nothing in return, why make the change? Without the compelling reason
to do it, all this
is jus
Hi Ivan,
not sure that would be an optimization, since you'd trade a conditional
write for an unconditional one. The computation itself for the empty
string has trivial cost.
/Claes
On 2019-04-08 18:12, Ivan Gerasimov wrote:
Hi Claes!
Would it make sense to preset hashIsZero = true in the emp
Hi Claes!
Would it make sense to preset hashIsZero = true in the empty string
constructor?
The current code avoids calculating the hashCode for an empty string,
and the new code doesn't seem to do that because hashIsZero = false by
default for each newly constructed copy of the empty string.
Right, this and possibly reducing latency when running with String
deduplication enabled might be the more tangible benefits. Removing
a cause for spurious performance degradations is nice, but mainly
theoretical. There's likely a pre-existing negative interaction
between string dedup and String a
On 08/04/2019 15:55, Aleksey Shipilev wrote:
> Why introduce this complexity to begin with? That's my argument. The
> complication of this code gives
> us almost nothing in return, why make the change? Without the compelling
> reason to do it, all this
> is just a runaway "hold my beer" micro-opt
On 4/8/19 2:44 PM, Peter Levart wrote:
> If you're afraid that a future maintainer of that code would not realize
> that, then a simple comment
> put into String.hashCode method and java_lang_String::set_hash C++ metohd
> that would say something
> like the following:
>
> // only a single field
On 4/8/19 1:40 PM, Aleksey Shipilev wrote:
On 4/8/19 1:28 PM, Peter Levart wrote:
The reasoning is very similar as with just one field. With one field (hash) the
thread sees either
the default value (0) or a non-zero value calculated either by this thread
sometime before or by a
concurrent
On 4/8/19 1:28 PM, Peter Levart wrote:
> On 4/8/19 12:28 PM, Aleksey Shipilev wrote:
>>> However, you also said in your opening criticism
>>>
>>> "I had hard time convincing myself that code is concurrency-safe"
>>>
>>> I think that is a more telling complaint. Can you elaborate on why you
>>> fo
On 4/8/19 12:28 PM, Aleksey Shipilev wrote:
However, you also said in your opening criticism
"I had hard time convincing myself that code is concurrency-safe"
I think that is a more telling complaint. Can you elaborate on why you
found it hard to convince yourself of this? (I know what I
On 4/8/19 1:00 PM, Claes Redestad wrote:
> On 2019-04-08 12:28, Aleksey Shipilev wrote:
>> Because the whole thing in current code is "benign data race" on hash field.
>> Pulling in another
>> field into race needs careful consideration if it breaks the benignity. It
>> apparently does not, but
>
On 2019-04-08 12:28, Aleksey Shipilev wrote:
Because the whole thing in current code is "benign data race" on hash field.
Pulling in another
field into race needs careful consideration if it breaks the benignity. It
apparently does not, but
the cognitive complexity involved in reading that code
On 2019-04-08 12:15, Andrew Dinn wrote:
Claes, is there a reason why you named the argument to method
hash_is_set 'string' when every other method uses the name
'java_string'? Is you 'j' key a tad sticky?
I took the cue from set_hash which uses the 'string' naming,
but yes, you're right to poin
On 4/8/19 12:15 PM, Andrew Dinn wrote:
> On 08/04/2019 10:42, Claes Redestad wrote:
>> On 2019-04-08 11:35, Aleksey Shipilev wrote:
Sure, String::hashCode/hash_code locally becomes a bit more complex, but
I view this as being a net improvement on the total amount of special
handling
I think the most benefit in this patch is the emptyString.hashCode()
speedup. By holding a boolean flag in the String object itself, there is
one less de-reference to be made on fast-path in case of empty string.
Which shows in microbenchmark and would show even more if code iterated
many diffe
On 08/04/2019 10:42, Claes Redestad wrote:
> On 2019-04-08 11:35, Aleksey Shipilev wrote:
>>> Sure, String::hashCode/hash_code locally becomes a bit more complex, but
>>> I view this as being a net improvement on the total amount of special
>>> handling we need to do for Strings and their hash co
On 2019-04-08 11:35, Aleksey Shipilev wrote:
Sure, String::hashCode/hash_code locally becomes a bit more complex, but
I view this as being a net improvement on the total amount of special
handling we need to do for Strings and their hash codes.
I don't see it. The change *added* new handling
On 4/8/19 11:25 AM, Claes Redestad wrote:
> On 2019-04-08 10:56, Aleksey Shipilev wrote:
>> Regardless, I think this change does not carry its weight. Introducing
>> special paths for handling
>> something as obscure as zero hash code, which then raises questions about
>> correctness (I had hard
On 2019-04-08 10:56, Aleksey Shipilev wrote:
Regardless, I think this change does not carry its weight. Introducing special
paths for handling
something as obscure as zero hash code, which then raises questions about
correctness (I had hard
time convincing myself that code is concurrency-safe
On 4/8/19 10:41 AM, Claes Redestad wrote:
> by adding a bit to String that is true iff String.hash has been calculated as
> being 0, we can get
> rid of the corner case where such hash
> codes are recalculated on every call.
>
> Peter Levart came up with a elegant scheme for ensuring that we can
Hi,
by adding a bit to String that is true iff String.hash has been
calculated as being 0, we can get rid of the corner case where such hash
codes are recalculated on every call.
Peter Levart came up with a elegant scheme for ensuring that we can keep
using non-volatile stores without explicit
32 matches
Mail list logo
