added observation points to track performance/resource usage of filter #7
Conversation
- ability to tell when collisions happen - get number of items in the filter - get memory usage of filter
pkaeding
force-pushed
the
pk/indicate-collision
branch
from
5a8e027 to
bbc6efc
Compare
go/oppobloom/oppobloom.go
Outdated
| contains = bytes.Equal(oldId, id) | ||
| collision = len(oldId) != 0 && !contains | ||
| if !contains && !collision { | ||
| atomic.AddUint32(&f.numEntries, 1) |
There was a problem hiding this comment.
Hm, all this code from here and below seems like it could live on a struct that wraps around this one if the ContainsCollision method was added.
I'd be more inclined to add just that ContainsCollision method and skip the rest of this code. In part, for better separation of concerns, but also because I don't think the numEntries code is quite what a lot of people need. (It seems to count the number of times a new entry was put in, sure, but its name suggests the number of items in the map at any given time)
There was a problem hiding this comment.
numEntries is actually meant to count the number of entries in the map-- it won't be incremented if there is a collision (in that case, there wouldn't be a new item added to the map, it would be replacing an item in the map).
There was a problem hiding this comment.
Good idea on the wrapper struct. I just pushed this up, if you'd like to take another look.
go/oppobloom/oppobloom.go
Outdated
| @@ -17,8 +17,10 @@ import ( | |||
| ) | |||
|
|
|||
| type Filter struct { | |||
| array []*[]byte | |||
| sizeMask uint32 | |||
| array []*[]byte | |||
There was a problem hiding this comment.
There's a bunch of gofmt changes in this code (it seems to predate my auto-gofmt set up!), so I've pushed up a gofmt change separately that now conflicts with this PR in parts.
Sorry but thanks for the heads up!
| return contains, collision | ||
| } | ||
|
|
||
| func (f *StandardFilter) containsCollisionOldVal(id []byte) (contains bool, collision bool, oldId []byte) { |
There was a problem hiding this comment.
Looks like this could be inlined to ContainsCollision, yeah?
| @@ -16,7 +16,13 @@ import ( | |||
| "unsafe" | |||
| ) | |||
|
|
|||
| type Filter struct { | |||
| type Filter interface { | |||
There was a problem hiding this comment.
I think this is better defined in the code users of the library write.
|
|
||
| // Like Filter, but has additional ability to expose the number of items currently | ||
| // in the filter, as well as the total size of the data held by the filter | ||
| type SizeTrackingFilter struct { |
There was a problem hiding this comment.
Ah, I was trying to leave this lib as small as possible and have you keep this code in your codebase. I'm not sure that's as general was we'd assume.
| } | ||
|
|
||
| func (f *Filter) Size() int { | ||
| func (f *StandardFilter) Size() int { |
We love this package, but we found ourselves want to track how it was performing. The plan is to hook our
go-metricscode into these, adding a counter for collisions, and functional gauges on the sizes of the filter.I realize that the number of items and memory usages are not totally atomic (ie, you can add an item to the set, and it could be reflected in the number of items, but not in the memory used), but over time, they should give the right trends, and every item should be counted.