improve hash calculation cache pool #1246
Conversation
Using potentially very large number for a mod is probably not very effective
And this is very easy to check
Seems like this may have odd sideeffects
|
Looks like this shaves ~100ms of load time on test app |
| @@ -140,7 +140,7 @@ final long approximateSize() { | |||
| this.loaderRef = loaderRef; | |||
| this.className = className; | |||
|
|
|||
| hashCode = (int) (31 * this.loaderHash) ^ className.hashCode(); | |||
| hashCode = 31 * this.loaderHash + className.hashCode(); | |||
There was a problem hiding this comment.
Nice -- did these two changes reduce the collision rate?
Mostly just curious
There was a problem hiding this comment.
I didn't check, but I did notice that before equals falled down to string comparison way too often.
There was a problem hiding this comment.
Yes, that might have more to do with the loader hash. I think the change of the bootstrap hash is good, but I'm not so sure about the other parts.
Here, I was using a variation on FNV: https://en.wikipedia.org/wiki/Fowler%E2%80%93Noll%E2%80%93Vo_hash_function
I suspect this can be improved, but I'm not so sure that switching from ^ to + is a good idea.
There was a problem hiding this comment.
I think + is used by java.util.Arrays, and 'promoted' by things like Guava (java.util.Arrays has a bit different formula, but in our case I think it will produce only constant difference).
As far as I can see FNV performs all calculation on bytes and then 'collects' result in int - ensuring mixing within values. I'm not sure how different this is from current implementation in this code. Also FNV uses 'carefully selected' prime and offset.
FWIW in my quick test on 1000 iterations of hash(r.nextInt() % 10, r.nextInt() % 100) new implementation routinely produces better results (less collisions) than old one.
|
|
||
| TypeCacheKey that = (TypeCacheKey) obj; | ||
| if (this == obj) { |
There was a problem hiding this comment.
Does this case get exercised? I believe the outer layer always creates a new TypeCacheKey, so this probably doesn't get exercised much.
I did try a separate branch where I used ImmutableTypeCacheKeys for put and used a thread local MutableTypeCacheKey for look-up. That did slightly reduce allocation but the allocation from evicting and then rematerializing still dominated and there was no measurable reduction in GCs or impact on start-up.
There was a problem hiding this comment.
I do not think I have direct data on this... would you like me to remove this?
There was a problem hiding this comment.
Yes, I don't think is helping -- probably hurting slightly.
There was a problem hiding this comment.
I think it will be optimized away eventually by JVM, but I've removed it anyway.
We have to check string equivalence regardless of classloader state
|
|
||
| if (loaderHash != that.loaderHash) return false; | ||
| if (hashCode != that.hashCode) { |
There was a problem hiding this comment.
I was mostly assuming that hashCode comparisons had already been done.
The idea behind the loaderHash comparison was that it provides a fast exit for some hash collisions.
| return false; | ||
| } | ||
|
|
||
| if (className.equals(that.className)) { |
There was a problem hiding this comment.
Yes, this part is debatable but still quite deliberate.
The loaderRef reference equivalence check was placed before the className.equals check because it is faster.
The order is reversed for the slow path because I want to avoid calling Reference.get whenever possible.
The reason being that concurrency GCs will "strengthen" the reference on a get call.
https://github.com/real-logic/agrona/blob/master/agrona/src/main/java/org/agrona/References.java has a nice explanation.
There was a problem hiding this comment.
I think performance wise this check is equivalent.
It was:
- equivalence check for references
- immediately after
equalson class name in both branches
Now:
- Equals in classname first
- equivalence check if first passes.
So if anything we save equivalence check in some cases. Strings were compared anyway.
There was a problem hiding this comment.
Yes, that's a fair point -- and if we put back the loaderHash check that covers a fast exit for loader mismatch.
There was a problem hiding this comment.
I've put that back
This reverts commit 50793e5.
mar-kolya
force-pushed
the
mar-kolya/improve-hash-calclulation-cache-pool
branch
from
57ba9df
to
82dd2aa
Compare
No description provided.