RFR: 8221836: Avoid recalculating String.hash when zero

[Claes Redestad]

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 just a runaway "hold my beer" micro-optimization exercise, which is fun and instructional, but
>> should not be pushed to the actual JDK. In other words, just because we *can* it does not follow
>> that we *should*.
> I think that is a good argument. The vast majority of apps are not going
> to see any performance hit from the relatively rare occurrence of zero
> hashes. Those rare apps which see a lot of them will still only see a
> marginal performance hit.
> 
> n.b. I say marginal because I believe that an app which sees enough zero
> hashes for the effect not to be marginal (i.e.it is not doing much else)
> is probably not an app but a Trojan horse of a (micro?) benchmark
> masquerading as an app (timeo Danaos et lecti marcae ferentes -- pardon
> my Latin and don't ask what those notches on the couch might mean ;-).
> 
> So, I agree with Aleksey that adding a potential maintenance headache
> for this little gain is not the right trade-off.

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.

Secondly, I think the gain w.r.t. defense-in-depth is very real. Sure,
we have alt-hashing and similar counter-measures in various JDK
collection libraries, but that is unlikely to help every API or library
ever invented out there.

Third... well, the other performance gains are of course nice-to-
have - improvements to "".hashCode() and allowing String deduplication
to not have to filter out such Strings - but I agree that they are
likely mostly theoretical for anything real-world.

If this change then exposes a bug in some unexpected place elsewhere
(I can only guess what dangers lurks out there... unforeseen interaction
with a weaker memory model? some wonky JIT reordering?) then that might
even be for the better in the end. If/when that happens, we can opt to
back this out (or not) while addressing whatever issue we've unearthed.

/Claes

[1] http://cr.openjdk.java.net/~redestad/8221836/open.02/