RFR: 8221836: Avoid recalculating String.hash when zero
shade at redhat.com
Tue Apr 9 08:11:51 UTC 2019
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 to yield: for example, accept this patch provisionally, if Claes and/or
accepting reviewers agree that any follow-up issue with it triggers the immediate backout, and
future attempts to introduce it are rejected given the observed non-trivial cost. This seems like a
no-brainer for those who argue there is little risk in doing this.)
> 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.
Yes, I am a firm believer that concurrency hacks need justifications, regardless of how simple they
appear at the moment. Call me paranoid, but I know enough about (Java) concurrency to be paranoid.
For tiny benefits, you need to demonstrate the non-existent cost to tip the cost/benefit balance
over the acceptance threshold. But even the benefits are questionable:
> Meanwhile, there are two reasons I want the change:
> 1. Less risk of spurious updates to COW memory segments in
> shared archives.
There is no risk for current code either. How come adding the new writable field provides "less risk
of updates", anyway? With one field we can track the object state in an obvious manner. Spreading
that state over two fields makes it less risky how exactly?
> 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 easier and much broader attack
on String hashcode. We don't get to wave in every single hack into class libraries under "security"
justification, especially when the mitigation already exists.
More information about the core-libs-dev