Repository navigation
Conversation
Inventory ids come from peers, so Item no longer derives its hash code from the id bytes directly. It computes SipHash-2-4 over the 32 id bytes once in the constructor, keyed by two SecureRandom values drawn when the class loads. equals() and Sha256Hash.hashCode() are unchanged.
Inventory ids come from peers, so Item no longer derives its hash code from the id bytes directly. It computes SipHash-2-4 over the 32 id bytes once in the constructor, keyed by two SecureRandom values drawn when the class loads. equals() and Sha256Hash.hashCode() are unchanged.
|
|
||
| private static HashFunction newIdHash() { | ||
| SecureRandom random = new SecureRandom(); | ||
| return Hashing.sipHash24(random.nextLong(), random.nextLong()); |
There was a problem hiding this comment.
Could you share the benchmark code and clarify whether the timings include Item construction? SipHash is computed in the constructor, so cache operations on prebuilt items would miss that added cost.
Please also compare construction plus cache lookup/insertion, including allocation and GC overhead, using the same workload and JVM warmup before and after the change. Results on both JDK 8/x86_64 and JDK 17/aarch64 would help assess the performance impact.
There was a problem hiding this comment.
The timings in the description include construction: new Item(...) is inside the timed loop, and only id generation is outside. They were single cold runs, though, so I reran a warmed before/after comparison (code below).
Base 37ea60a8bc vs this PR, each in its own JVM; 5 × 1 s warmup and 5 × 1 s measurement; JDK 17.0.18 / aarch64, -XX:+UseZGC -Xms2g -Xmx2g. Allocation is the bytes allocated by the measuring thread per item.
| Per item | Before | This PR |
|---|---|---|
new Item(...) |
22.6–23.1 ns, 40 B | 46.4–48.2 ns, 136 B |
| Construction + cache lookups/insertions (INVENTORY path) | 671–1494 ns, 447 B | 774–1272 ns, 543 B¹ |
¹ 695 B in one of the five JVMs.
Construction costs about 24 ns and 96 B more per item, because Guava's hashBytes allocates a hasher, a buffer and the HashCode. With the cache operations included, allocation is 21% higher and the time difference is within run-to-run variation. On that path ZGC ran 8 times per 5 s measurement before and 4–8 times with this PR.
I don't have a JDK 8 / x86_64 machine at hand, so there are no numbers for it yet; the harness runs there unchanged.
hash, type and time are assigned only in the constructor.
What does this PR do?
Changes
Item.hashCode()to a SipHash-2-4 hash of the full 32-byte id, keyed by twoSecureRandomvalues drawn once per process. The hash is computed once in the constructor and cached in a final field.Item.equals(),Sha256Hash.hashCode()and all callers are unchanged.Why are these changes required?
Itemis the key of the inventory caches and maps inAdvServiceandPeerConnection. A keyed hash over the full id gives these collections a uniform key distribution. Computing it once keeps lookups in the periodic fetch and spread loops as cheap as before.This PR has been tested by:
ItemTest:Itembuilt from aBlockIdequals one built from aSha256Hashwith the same bytes, and the two share a hash code../gradlew :framework:test --tests "org.tron.core.net.*": 361 tests in 41 classes, all passed (JDK 17 / aarch64)../gradlew :framework:checkstyleMain :framework:checkstyleTest: passed.Follow up
None.
Extra details
Itemis only an in-memory key in the network layer. It is never persisted, serialized, or compared across processes, so a per-process key does not affect behavior between nodes.Sha256Hash.equals()andBlockId.equals()both compare id bytes, and the new hash is computed from the same bytes, so theequals/hashCodecontract holds.Item-keyed maps.Hashingis already used in the project.