Add Iterator to iterate over keys #22

wendigo · 2016-10-12T16:31:47Z

No description provided.

coveralls · 2016-10-12T16:32:45Z

Coverage increased (+0.1%) to 98.773% when pulling e2823d8 on wendigo:master into f403062 on allegro:master.

coveralls · 2016-10-12T17:53:10Z

Coverage increased (+0.1%) to 98.784% when pulling 5b5116b on wendigo:master into f403062 on allegro:master.

wendigo · 2016-10-12T21:32:06Z

bigcache.go

+type EntryInfoFilter func(EntryInfo) bool
+
+// EntryInfoIterator allows to iterate over entries in the cache
+type EntryInfoIterator chan EntryInfo


I would like this iterator to be as non-blocking as possible

With channel you add blocking. Take a look at loops you created. You are adding mutex for reading from map and then pushing value to channel which is blocking. If you remove channel and instead return closure to function that returns entry form shard i on position index then you don't need to copy all (1024) elements and this will be a lazy iterator.

Channel is buffered

coveralls · 2016-10-14T12:59:06Z

Coverage decreased (-0.1%) to 98.567% when pulling ce30cf9 on wendigo:master into f403062 on allegro:master.

coveralls · 2016-10-14T13:06:41Z

Coverage decreased (-0.1%) to 98.567% when pulling f0c671f on wendigo:master into f403062 on allegro:master.

janisz · 2016-10-14T13:19:14Z

bigcache.go

+	cache        *BigCache
+	currentShard int
+	currentIndex int32
+	elements     []*EntryInfo


This could be just slice of indexes []uin32. We don't need all data during iteration.

janisz · 2016-10-14T13:21:47Z

bigcache.go

+	shard.lock.RLock()
+	defer shard.lock.RUnlock()
+
+	elements := make([]*EntryInfo, 0)


Try to estimate initial size for elements as len(shard.queue)/cache.config.MaxEntrySize

janisz · 2016-10-14T13:25:17Z

bigcache_test.go

+	for iterator.Next() {
+		current, err := iterator.Value()
+
+		if err == nil {


Add test when error occures: modify map when it terates, delete element under Value()

It also needs bench test.

Benchmark - done.

janisz · 2016-10-14T13:42:04Z

bigcache.go

+
+	if entry, err := it.cache.shards[it.currentShard].entries.Get(int(current.index)); err != nil {
+		return nil, fmt.Errorf("Could not retrieve entry from cache")
+	} else {


if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (golint)

janisz · 2016-10-14T13:43:22Z

bigcache.go

@@ -33,6 +33,79 @@ type cacheShard struct {
 	onRemove    func(wrappedEntry []byte)
 }

+// EntryInfo holds informations about entry in the cache
+type EntryInfo struct {


Why not contain entry value?

What is the reason to keep encoded entry instead of parsed?

Reason is simple: entryinfo is lazy - you will only parse what you need instead of everything

Got it but it's lazy every time it's called instead of loading values on a first call. I think parsing data should not be much slower then copying them but it should be checked.

So whats whit this? I did a small benchmark and gready extraction is a little bit slower than copy.

diff --git a/bigcache.go b/bigcache.go index d610216..f15c1de 100644 --- a/bigcache.go +++ b/bigcache.go @@ -35,27 +35,30 @@ type cacheShard struct { // EntryInfo holds informations about entry in the cache type EntryInfo struct { - entry []byte + timestamp uint64 + hash uint64 + key string + value []byte } // Key returns entry's underlying key func (e EntryInfo) Key() string { - return readKeyFromEntry(e.entry) + return e.key } // Hash returns entry's hash value func (e EntryInfo) Hash() uint64 { - return readHashFromEntry(e.entry) + return e.hash } // Timestamp returns entry's timestamp (time of insertion) func (e EntryInfo) Timestamp() uint64 { - return readTimestampFromEntry(e.entry) + return e.timestamp } // Value returns entry's underlying value func (e EntryInfo) Value() []byte { - return readEntry(e.entry) + return e.value } // EntryInfoIterator allows to iterate over entries in the cache @@ -140,11 +143,11 @@ func (it *EntryInfoIterator) Value() (EntryInfo, error) { return EntryInfo{}, fmt.Errorf("Could not retrieve entry from cache") } - var dst = make([]byte, len(entry)) - copy(dst, entry) - return EntryInfo{ - entry: dst, + timestamp: readTimestampFromEntry(entry), + hash: readHashFromEntry(entry), + key: readKeyFromEntry(entry), + value: readEntry(entry), }, nil }

$go test ./... -bench=BenchmarkIterateOverCache -run=NONE -count 3 -cpu 1,6 -benchmem > new.txt && git stash && go test ./... -bench=BenchmarkIterateOverCache -run=NONE -count 3 -cpu 1,6 benchmem > old.txt $ benchcmp old.txt new.txt benchmark old ns/op new ns/op delta BenchmarkIterateOverCache/512-shards 224 224 +0.00% BenchmarkIterateOverCache/512-shards 217 223 +2.76% BenchmarkIterateOverCache/512-shards 218 248 +13.76% BenchmarkIterateOverCache/512-shards-6 273 293 +7.33% BenchmarkIterateOverCache/512-shards-6 273 284 +4.03% BenchmarkIterateOverCache/512-shards-6 274 284 +3.65% BenchmarkIterateOverCache/1024-shards 214 216 +0.93% BenchmarkIterateOverCache/1024-shards 214 215 +0.47% BenchmarkIterateOverCache/1024-shards 215 218 +1.40% BenchmarkIterateOverCache/1024-shards-6 268 279 +4.10% BenchmarkIterateOverCache/1024-shards-6 271 278 +2.58% BenchmarkIterateOverCache/1024-shards-6 268 281 +4.85% BenchmarkIterateOverCache/8192-shards 209 210 +0.48% BenchmarkIterateOverCache/8192-shards 207 210 +1.45% BenchmarkIterateOverCache/8192-shards 207 211 +1.93% BenchmarkIterateOverCache/8192-shards-6 265 276 +4.15% BenchmarkIterateOverCache/8192-shards-6 266 275 +3.38% BenchmarkIterateOverCache/8192-shards-6 263 279 +6.08%

What with this?

Ok, done, it saves some allocs (20B/op vs 32B/op). Times are the a little smaller.

coveralls · 2016-10-14T18:39:40Z

Coverage increased (+0.2%) to 98.873% when pulling 50a2570 on wendigo:master into f403062 on allegro:master.

coveralls · 2016-10-14T19:11:32Z

Coverage decreased (-2.2%) to 96.458% when pulling 4e08def on wendigo:master into f403062 on allegro:master.

coveralls · 2016-10-14T19:15:31Z

Coverage increased (+0.2%) to 98.91% when pulling 1784e63 on wendigo:master into f403062 on allegro:master.

coveralls · 2016-10-15T21:29:01Z

Coverage increased (+0.2%) to 98.91% when pulling b037a2a on wendigo:master into f403062 on allegro:master.

coveralls · 2016-10-16T11:57:40Z

Coverage increased (+0.3%) to 98.928% when pulling edc1ec7 on wendigo:master into f403062 on allegro:master.

janisz · 2016-10-17T16:49:10Z

bigcache.go

+}
+
+// Value returns current value from the iterator
+func (it *EntryInfoIterator) Value() (*EntryInfo, error) {


I think we should return value not a pointer to make it obvious that we return copy of underlying value.

janisz · 2016-10-17T17:14:14Z

bigcache.go

+
+		// partition is empty - move to next
+		if len(it.elements) == 0 {
+			goto incrementShard


Just use for instead of if above the label

Why? :) I don't understand the pros

I mean change if in line 90 (above the label) and remove this condition. You are implementing for loop with if and goto

Yep, that's exactly what I'm doing and code is readable for me. Goto is a standard jmp in the code

Nevermind - I rewrite this part of iterator

I'd prefer to use for loop and extract condition to well named function so the comment will be redundant.

coveralls · 2016-10-18T10:27:48Z

Coverage increased (+0.2%) to 98.925% when pulling 60e3989 on wendigo:master into f403062 on allegro:master.

janisz · 2016-10-19T14:18:51Z

bigcache.go

+func (it *EntryInfoIterator) Value() (EntryInfo, error) {
+	it.Lock()
+	defer it.Unlock()
+	current := it.elements[it.currentIndex]


I think we should check if current index is valid.

janisz · 2016-10-19T14:19:35Z

bigcache.go

+
+		for i := it.currentShard + 1; i < it.cache.config.Shards; i++ {
+			it.currentShard, it.currentIndex, it.elements = i, 0, copyCurrentShardMap(it.cache.shards[i])
+


This line is too long.

janisz · 2016-10-19T14:21:19Z

bigcache.go

+
+	it.currentIndex++
+
+	if it.currentIndex >= len(it.elements) {


How about reverting condition. Just for readability.

You mean: if !(len(it.elements) > it.currentIndex) ?

janisz · 2016-10-19T14:22:31Z

bigcache.go

+			return false
+		}
+
+		for i := it.currentShard + 1; i < it.cache.config.Shards; i++ {


What if all shards are empty?

It will iterate to the end and return false

janisz · 2016-10-19T14:26:45Z

bigcache.go

+	return elements
+}
+
+// Next returns true if there is next element in the iterator


This describes only half of the job that Next() do becouse it also moves iterator to next element or invalid position if it doesn't exist. This comment better suits HasNext()

Yeah, you're right - changed to HasNext()

coveralls · 2016-10-19T15:04:30Z

Coverage increased (+0.3%) to 98.95% when pulling 183ce8d on wendigo:master into f403062 on allegro:master.

janisz

Almost ready. Only small fixes need to be done. We need to decide how EntryInfo should look like. After introducing it it will be recommended to us it as a default type instead of []byte.
@druminski @medzin Can you take a look at this?

janisz · 2016-10-20T17:05:47Z

bigcache.go

+	it.valid = false
+	it.currentIndex++
+
+	if !(len(it.elements) > it.currentIndex) {


Oh, I was thinking about early return true so the code below doesn't need to be indent.

if len(it.elements) > it.currentIndex { it.valid = true; return true }

+1 it also simplifies logical statement as negation mark is not necessary

Done :) Renamed HasNext to SetNext also

janisz · 2016-10-20T17:22:54Z

bigcache_test.go

+	assert.Equal(t, current.Key(), "key")
+	assert.Equal(t, current.Hash(), uint64(0x3dc94a19365b10ec))
+	assert.Equal(t, current.Value(), []byte("value"))
+	assert.Equal(t, current.Timestamp(), uint64(0))


Parameters order mismatch func Equal(t TestingT, expected, actual interface{}, msgAndArgs ...interface{}) bool. Check with other places too.

janisz · 2016-10-20T17:44:30Z

bigcache.go

@@ -33,6 +33,79 @@ type cacheShard struct {
 	onRemove    func(wrappedEntry []byte)
 }

+// EntryInfo holds informations about entry in the cache
+type EntryInfo struct {


So whats whit this? I did a small benchmark and gready extraction is a little bit slower than copy.

diff --git a/bigcache.go b/bigcache.go index d610216..f15c1de 100644 --- a/bigcache.go +++ b/bigcache.go @@ -35,27 +35,30 @@ type cacheShard struct { // EntryInfo holds informations about entry in the cache type EntryInfo struct { - entry []byte + timestamp uint64 + hash uint64 + key string + value []byte } // Key returns entry's underlying key func (e EntryInfo) Key() string { - return readKeyFromEntry(e.entry) + return e.key } // Hash returns entry's hash value func (e EntryInfo) Hash() uint64 { - return readHashFromEntry(e.entry) + return e.hash } // Timestamp returns entry's timestamp (time of insertion) func (e EntryInfo) Timestamp() uint64 { - return readTimestampFromEntry(e.entry) + return e.timestamp } // Value returns entry's underlying value func (e EntryInfo) Value() []byte { - return readEntry(e.entry) + return e.value } // EntryInfoIterator allows to iterate over entries in the cache @@ -140,11 +143,11 @@ func (it *EntryInfoIterator) Value() (EntryInfo, error) { return EntryInfo{}, fmt.Errorf("Could not retrieve entry from cache") } - var dst = make([]byte, len(entry)) - copy(dst, entry) - return EntryInfo{ - entry: dst, + timestamp: readTimestampFromEntry(entry), + hash: readHashFromEntry(entry), + key: readKeyFromEntry(entry), + value: readEntry(entry), }, nil }

$go test ./... -bench=BenchmarkIterateOverCache -run=NONE -count 3 -cpu 1,6 -benchmem > new.txt && git stash && go test ./... -bench=BenchmarkIterateOverCache -run=NONE -count 3 -cpu 1,6 benchmem > old.txt $ benchcmp old.txt new.txt benchmark old ns/op new ns/op delta BenchmarkIterateOverCache/512-shards 224 224 +0.00% BenchmarkIterateOverCache/512-shards 217 223 +2.76% BenchmarkIterateOverCache/512-shards 218 248 +13.76% BenchmarkIterateOverCache/512-shards-6 273 293 +7.33% BenchmarkIterateOverCache/512-shards-6 273 284 +4.03% BenchmarkIterateOverCache/512-shards-6 274 284 +3.65% BenchmarkIterateOverCache/1024-shards 214 216 +0.93% BenchmarkIterateOverCache/1024-shards 214 215 +0.47% BenchmarkIterateOverCache/1024-shards 215 218 +1.40% BenchmarkIterateOverCache/1024-shards-6 268 279 +4.10% BenchmarkIterateOverCache/1024-shards-6 271 278 +2.58% BenchmarkIterateOverCache/1024-shards-6 268 281 +4.85% BenchmarkIterateOverCache/8192-shards 209 210 +0.48% BenchmarkIterateOverCache/8192-shards 207 210 +1.45% BenchmarkIterateOverCache/8192-shards 207 211 +1.93% BenchmarkIterateOverCache/8192-shards-6 265 276 +4.15% BenchmarkIterateOverCache/8192-shards-6 266 275 +3.38% BenchmarkIterateOverCache/8192-shards-6 263 279 +6.08%

druminski · 2016-10-31T14:42:29Z

bigcache.go

+	it.valid = false
+	it.currentIndex++
+
+	if !(len(it.elements) > it.currentIndex) {


+1 it also simplifies logical statement as negation mark is not necessary

druminski · 2016-10-31T14:56:58Z

bigcache.go

+}
+
+// HasNext returns true if there is next element in the iterator, false otherwise
+func (it *EntryInfoIterator) HasNext() bool {


This method name is misleading right now as its implementation does not only answer the question whether there is another element in iterator but also modifies currentIndex value. Because of that when you call time after time HasNext() then you can get different answers. I see two options here, we can change this method name to SetNext() bool or we can implement HasNext() to be idempotent and then Value() convert to Next().

You're right - renamed method to SetNext().

coveralls · 2016-11-02T20:43:45Z

Coverage increased (+0.01%) to 98.688% when pulling f007704 on wendigo:master into f403062 on allegro:master.

coveralls · 2016-11-02T21:34:18Z

Coverage increased (+0.03%) to 98.705% when pulling a20e201 on wendigo:master into f403062 on allegro:master.

coveralls · 2016-11-02T21:40:05Z

Coverage increased (+0.03%) to 98.705% when pulling d1439df on wendigo:master into f403062 on allegro:master.

coveralls · 2016-11-02T21:47:52Z

Coverage increased (+0.3%) to 98.956% when pulling d3bfe18 on wendigo:master into f403062 on allegro:master.

wendigo · 2016-11-02T21:54:39Z

Waiting for final thoughts @janisz

coveralls · 2016-11-02T22:07:25Z

Coverage increased (+0.3%) to 98.956% when pulling 216b1fe on wendigo:master into f403062 on allegro:master.

janisz · 2016-11-02T22:02:35Z

bigcache.go

+
+// ErrCannotRetrieveEntry is reported when entry cannot be retrieved from underlying
+var ErrCannotRetrieveEntry = errors.New("Could not retrieve entry from cache")
+


This two should not be public. Use empty struct withe Error method like we do in entry_not_found_error.go or make this private. Take a look at http://dave.cheney.net/2016/04/07/constant-errors

What do you mean "What with this"? :)

Not this line but I assume you are referring to this discussion #22 (comment)

janisz · 2016-11-02T22:04:08Z

bigcache.go

@@ -33,6 +33,79 @@ type cacheShard struct {
 	onRemove    func(wrappedEntry []byte)
 }

+// EntryInfo holds informations about entry in the cache
+type EntryInfo struct {


What with this?

janisz · 2016-11-02T22:11:13Z

bigcache_bench_test.go

@@ -17,6 +17,8 @@ func BenchmarkWriteToCacheWith1Shard(b *testing.B) {
 func BenchmarkWriteToLimitedCacheWithSmallInitSizeAnd1Shard(b *testing.B) {
 	m := blob('a', 1024)
 	cache, _ := NewBigCache(Config{1, 100 * time.Second, 100, 256, false, nil, 1, nil})
+
+	b.ReportAllocs()


If applied to all tests then better use -test.benchmem

It will include setup also (creating entries for reads etc)

janisz · 2016-11-02T22:12:33Z

bigcache_test.go

+	t.Parallel()
+
+	// given
+	keysCount := 10000


Why do we need so many entries in unit test?

Reduced by 10x

coveralls · 2016-11-02T23:12:43Z

Coverage decreased (-0.5%) to 98.172% when pulling eeaf0df on wendigo:master into f403062 on allegro:master.

coveralls · 2016-11-02T23:16:24Z

Coverage increased (+0.3%) to 98.956% when pulling aa2187b on wendigo:master into f403062 on allegro:master.

Add Iterator to iterate over keys

Add ability to iterate over keys

e2823d8

Add filtering

5b5116b

wendigo changed the title ~~Add Keys() function to iterate over keys~~ Add EntriesMatching() function to iterate over keys Oct 12, 2016

wendigo commented Oct 12, 2016

View reviewed changes

Next attempt

ce30cf9

wendigo changed the title ~~Add EntriesMatching() function to iterate over keys~~ Add Iterator to iterate over keys Oct 14, 2016

Remove unused type

f0c671f

janisz requested changes Oct 14, 2016

View reviewed changes

More iterator tests

50a2570

Benchmarks

4e08def

Mateusz Gajewski and others added 2 commits October 14, 2016 21:16

Tests

1784e63

More benchmarks

b037a2a

Make iterator safe to use concurrently

edc1ec7

janisz reviewed Oct 17, 2016

View reviewed changes

Rewrite goto loop

60e3989

janisz requested changes Oct 19, 2016

View reviewed changes

After review fixes

183ce8d

janisz requested changes Oct 20, 2016

View reviewed changes

druminski reviewed Oct 31, 2016

View reviewed changes

Simplify logic

f007704

druminski approved these changes Nov 2, 2016

View reviewed changes

Describe errors, gain some ns

a20e201

Beautify tests

d1439df

Mateusz Gajewski added 2 commits November 2, 2016 22:44

Remove redundant if clause

ead0943

Simplify code

d3bfe18

Report allocs

216b1fe

janisz requested changes Nov 2, 2016

View reviewed changes

Janisz review fixes

eeaf0df

janisz approved these changes Nov 2, 2016

View reviewed changes

Coverage

aa2187b

wendigo merged commit 8c40d09 into allegro:master Nov 2, 2016

siennathesane mentioned this pull request Oct 11, 2017

Update release notes for v1.0.0 #52

Closed

flisky pushed a commit to flisky/bigcache that referenced this pull request May 7, 2020

Merge pull request allegro#22 from wendigo/master

7424a0e

Add Iterator to iterate over keys


		for i := it.currentShard + 1; i < it.cache.config.Shards; i++ {
		it.currentShard, it.currentIndex, it.elements = i, 0, copyCurrentShardMap(it.cache.shards[i])


		// ErrCannotRetrieveEntry is reported when entry cannot be retrieved from underlying
		var ErrCannotRetrieveEntry = errors.New("Could not retrieve entry from cache")

Add Iterator to iterate over keys #22

Add Iterator to iterate over keys #22

Conversation

wendigo commented Oct 12, 2016

coveralls commented Oct 12, 2016

coveralls commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 14, 2016

coveralls commented Oct 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 14, 2016

coveralls commented Oct 14, 2016

coveralls commented Oct 14, 2016

coveralls commented Oct 15, 2016

coveralls commented Oct 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 19, 2016

janisz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 2, 2016

coveralls commented Nov 2, 2016

coveralls commented Nov 2, 2016

coveralls commented Nov 2, 2016

wendigo commented Nov 2, 2016

coveralls commented Nov 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 2, 2016

coveralls commented Nov 2, 2016