Refactor hashType: []byte -> uint64 #228
Conversation
Codecov Report
@@ Coverage Diff @@
## main #228 +/- ##
==========================================
- Coverage 60.96% 60.86% -0.11%
==========================================
Files 66 66
Lines 3784 3774 -10
==========================================
- Hits 2307 2297 -10
Misses 1336 1336
Partials 141 141
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
|
||
| func (c *connType) getHash() totalHashType { | ||
| return copyTotalHash(c.hash) | ||
| return c.hash |
There was a problem hiding this comment.
Since this is a private type and attribute, maybe the getHash() method is not needed anymore and you can invoke directly c.hash
There was a problem hiding this comment.
I want the API of connection to have the property that once a connection object is created, its hash is read-only.
The other types that interact with connection (e.g. conntrackImpl and aggregators) are doing so using the connection interface rather than the connType struct. So in the current implementation I can't remove the method.
|
|
||
| func uint64ToBytes(data uint64) []byte { | ||
| b := make([]byte, 8) | ||
| binary.BigEndian.PutUint64(b, data) |
There was a problem hiding this comment.
I guess the encoding order (little vs big endian) does not matter here, right?
ronensc
force-pushed
the
conntrack-refactor-hashtype
branch
from
9dd7555 to
cb092ec
Compare
4 participants
This PR changes the type under the hood of the hash from
[]bytetouint64.uint64simplifies the code since its easier to copy than[]byteand can serve as a key in a map (in contrast to[]byte)This PR is a follow up on the 2nd point from this review: #220 (comment)
I also changed the hash to use 64 bits instead of 32 bits to reduce the chance of collisions (different connections with the same hash).
Assuming a perfect hash function with uniform distribution of the has values,
for 32 bits we need only 9,300 connections to have 1% chance for collision.
But, for 64 bits we need ~61,000,000 connections to have 1% chance for collision.
I based this on the table from Birthday attack. @kalman Am I correct with my analysis?