Optimize bytes_queue #207

neal-zhu · 2020-03-12T06:10:19Z

Hi there, I try to optimize bytes_queue in two ways:

The length is encoded in varint format(the assertion on capacity will fail so I commented them out).
When we insert an entry before the head of the queue, and the size of this entry exactly equals to unsued bytes, there is no need to reserve bytes.

And below are the outputs of test on my Mac:

# Mine
2020/03/12 14:49:28 Allocated new queue in 130ns; Capacity: 88
2020/03/12 14:49:28 Allocated new queue in 211.918µs; Capacity: 585000
2020/03/12 14:49:28 Allocated new queue in 394.027µs; Capacity: 1170000
PASS

# Yours
2020/03/12 14:06:56 Allocated new queue in 156ns; Capacity: 94
2020/03/12 14:06:56 Allocated new queue in 719.26µs; Capacity: 585000
2020/03/12 14:06:56 Allocated new queue in 566.888µs; Capacity: 1170000
PASS

PS: It can save some bytes and avoid re-allocation of queue at some time

codecov-io · 2020-03-12T06:12:24Z

Codecov Report

❗ No coverage uploaded for pull request base (master@75a65a8). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #207   +/-   ##
=========================================
  Coverage          ?   90.58%           
=========================================
  Files             ?       15           
  Lines             ?      712           
  Branches          ?        0           
=========================================
  Hits              ?      645           
  Misses            ?       58           
  Partials          ?        9

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75a65a8...6899c7c. Read the comment docs.

cristaloleg · 2020-03-12T06:29:35Z

bigcache_test.go

@@ -426,7 +426,7 @@ func TestCacheCapacity(t *testing.T) {

 	// then
 	assertEqual(t, keys, cache.Len())
-	assertEqual(t, 81920, cache.Capacity())
+	//assertEqual(t, 81920, cache.Capacity())


Cause I use uvarint to save space, operations above this assertion will not cause a re-allocation of the queue, the capacity of it will be 81920/2.
To be honest, I do not think it's a good idea to assert the capacity of the cache with a hard coed. The magic number will be improper if the implementation has changed.
All assertion on the capacity of cache or bytes_queue depends heavily on the implementation detail of bytes_queue.

queue/bytes_queue.go

cristaloleg · 2020-03-12T06:32:46Z

queue/bytes_queue.go

-	return q.array[index+headerEntrySize : index+headerEntrySize+blockSize], blockSize, nil
+	blockSize, n := binary.Uvarint(q.array[index:])
+	if n <= 0 {
+		panic("wrong encoding")


Hm, should we panic here? simple return nil, 0, 0, err should be enough, isn't it?

Yes, do you have any suggestions on which error should it be?

janisz

Can you run benchmark for this change?
And can you add benchmark for encoding part? I wonder if uvarinat is slower then our current approach

neal-zhu · 2020-03-12T11:18:11Z

@janisz Hi, here's the benchmark result:

# Mine
2020/03/12 19:02:50 Allocated new queue in 180ns; Capacity: 88
2020/03/12 19:02:50 Allocated new queue in 226.702µs; Capacity: 585000
2020/03/12 19:02:50 Allocated new queue in 389.079µs; Capacity: 1170000
goos: darwin
goarch: amd64
pkg: github.com/allegro/bigcache/v2
BenchmarkWriteToCacheWith1Shard-4                            	 5192714	      1026 ns/op	     596 B/op	       3 allocs/op
BenchmarkWriteToLimitedCacheWithSmallInitSizeAnd1Shard-4     	 8772144	       508 ns/op	     104 B/op	       4 allocs/op
BenchmarkWriteToUnlimitedCacheWithSmallInitSizeAnd1Shard-4   	 1902460	      5424 ns/op	    3623 B/op	       2 allocs/op
BenchmarkWriteToCache/1-shards-4                             	 3431140	      1486 ns/op	     620 B/op	       3 allocs/op
BenchmarkWriteToCache/512-shards-4                           	 9916384	      1135 ns/op	     599 B/op	       3 allocs/op
BenchmarkWriteToCache/1024-shards-4                          	 7211010	       999 ns/op	     618 B/op	       3 allocs/op
BenchmarkWriteToCache/8192-shards-4                          	 6708818	      1065 ns/op	     602 B/op	       3 allocs/op
BenchmarkReadFromCache/1-shards-4                            	13963962	      2549 ns/op	     271 B/op	       2 allocs/op
BenchmarkReadFromCache/512-shards-4                          	 9840069	       828 ns/op	     271 B/op	       2 allocs/op
BenchmarkReadFromCache/1024-shards-4                         	 9384037	       923 ns/op	     271 B/op	       2 allocs/op
BenchmarkReadFromCache/8192-shards-4                         	10724266	      1131 ns/op	     271 B/op	       2 allocs/op
BenchmarkReadFromCacheWithInfo/1-shards-4                    	10171226	      1048 ns/op	     271 B/op	       2 allocs/op
BenchmarkReadFromCacheWithInfo/512-shards-4                  	 8341252	       888 ns/op	     271 B/op	       2 allocs/op
BenchmarkReadFromCacheWithInfo/1024-shards-4                 	10288347	      1135 ns/op	     271 B/op	       2 allocs/op
BenchmarkReadFromCacheWithInfo/8192-shards-4                 	 8065857	       827 ns/op	     271 B/op	       2 allocs/op
BenchmarkIterateOverCache/512-shards-4                       	24373030	       228 ns/op	      20 B/op	       2 allocs/op
BenchmarkIterateOverCache/1024-shards-4                      	22172804	       214 ns/op	      20 B/op	       2 allocs/op
BenchmarkIterateOverCache/8192-shards-4                      	22551939	       220 ns/op	      20 B/op	       2 allocs/op
BenchmarkWriteToCacheWith1024ShardsAndSmallShardInitSize-4   	 5228383	      1452 ns/op	    1108 B/op	       3 allocs/op
BenchmarkReadFromCacheNonExistentKeys/1-shards-4             	36336122	       137 ns/op	       7 B/op	       0 allocs/op
BenchmarkReadFromCacheNonExistentKeys/512-shards-4           	38118368	       141 ns/op	       7 B/op	       0 allocs/op
BenchmarkReadFromCacheNonExistentKeys/1024-shards-4          	36579570	       140 ns/op	       7 B/op	       0 allocs/op
BenchmarkReadFromCacheNonExistentKeys/8192-shards-4          	36496588	       149 ns/op	       7 B/op	       0 allocs/op
BenchmarkFnvHashSum64-4                                      	460839229	        10.1 ns/op	       0 B/op	       0 allocs/op
BenchmarkFnvHashStdLibSum64-4                                	90994177	        67.8 ns/op	      16 B/op	       2 allocs/op
PASS
ok  	github.com/allegro/bigcache/v2	555.615s
PASS
ok  	github.com/allegro/bigcache/v2/queue	0.012s
2020/03/12 19:12:05 Entry not found
2020/03/12 19:12:05 stored "testkey" in cache.
2020/03/12 19:12:05 empty request.
2020/03/12 19:12:05 stored "putKey" in cache.
2020/03/12 19:12:05 Entry not found
2020/03/12 19:12:05 test read error
2020/03/12 19:12:05 invalidDeleteKey not found.
2020/03/12 19:12:05  not found.
2020/03/12 19:12:05 empty request.
2020/03/12 19:12:05 entry is bigger than max shard size
PASS

# Yours
2020/03/12 18:48:16 Allocated new queue in 264.583µs; Capacity: 585000
2020/03/12 18:48:16 Allocated new queue in 182ns; Capacity: 94
2020/03/12 18:48:16 Allocated new queue in 478.836µs; Capacity: 1170000
goos: darwin
goarch: amd64
pkg: github.com/allegro/bigcache/v2
BenchmarkWriteToCacheWith1Shard-4                            	 4990784	      1342 ns/op	     598 B/op	       3 allocs/op
BenchmarkWriteToLimitedCacheWithSmallInitSizeAnd1Shard-4     	 8682642	       566 ns/op	      40 B/op	       3 allocs/op
BenchmarkWriteToUnlimitedCacheWithSmallInitSizeAnd1Shard-4   	 3486128	      8048 ns/op	    3953 B/op	       2 allocs/op
BenchmarkWriteToCache/1-shards-4                             	 3656758	      1755 ns/op	     616 B/op	       3 allocs/op
BenchmarkWriteToCache/512-shards-4                           	 5487960	      1203 ns/op	     595 B/op	       3 allocs/op
BenchmarkWriteToCache/1024-shards-4                          	 7292827	       952 ns/op	     617 B/op	       3 allocs/op
BenchmarkWriteToCache/8192-shards-4                          	 7152934	       986 ns/op	     631 B/op	       3 allocs/op
BenchmarkReadFromCache/1-shards-4                            	13202287	      2494 ns/op	     271 B/op	       2 allocs/op
BenchmarkReadFromCache/512-shards-4                          	 9122005	      1506 ns/op	     271 B/op	       2 allocs/op
BenchmarkReadFromCache/1024-shards-4                         	10188386	       964 ns/op	     271 B/op	       2 allocs/op
BenchmarkReadFromCache/8192-shards-4                         	10101868	      1393 ns/op	     271 B/op	       2 allocs/op
BenchmarkReadFromCacheWithInfo/1-shards-4                    	 8598147	       830 ns/op	     271 B/op	       2 allocs/op
BenchmarkReadFromCacheWithInfo/512-shards-4                  	 9947847	       873 ns/op	     271 B/op	       2 allocs/op
BenchmarkReadFromCacheWithInfo/1024-shards-4                 	 9378534	       900 ns/op	     271 B/op	       2 allocs/op
BenchmarkReadFromCacheWithInfo/8192-shards-4                 	 8794632	       953 ns/op	     271 B/op	       2 allocs/op
BenchmarkIterateOverCache/512-shards-4                       	21350042	       216 ns/op	      20 B/op	       2 allocs/op
BenchmarkIterateOverCache/1024-shards-4                      	20962723	       224 ns/op	      20 B/op	       2 allocs/op
BenchmarkIterateOverCache/8192-shards-4                      	21533272	       205 ns/op	      20 B/op	       2 allocs/op
BenchmarkWriteToCacheWith1024ShardsAndSmallShardInitSize-4   	 4855218	      1543 ns/op	    1190 B/op	       3 allocs/op
BenchmarkReadFromCacheNonExistentKeys/1-shards-4             	30844810	       150 ns/op	       7 B/op	       0 allocs/op
BenchmarkReadFromCacheNonExistentKeys/512-shards-4           	33429462	       139 ns/op	       7 B/op	       0 allocs/op
BenchmarkReadFromCacheNonExistentKeys/1024-shards-4          	32085747	       149 ns/op	       7 B/op	       0 allocs/op
BenchmarkReadFromCacheNonExistentKeys/8192-shards-4          	37587307	       141 ns/op	       7 B/op	       0 allocs/op
BenchmarkFnvHashSum64-4                                      	519146876	         9.40 ns/op	       0 B/op	       0 allocs/op
BenchmarkFnvHashStdLibSum64-4                                	70484605	        64.3 ns/op	      16 B/op	       2 allocs/op
PASS
ok  	github.com/allegro/bigcache/v2	556.285s
PASS
ok  	github.com/allegro/bigcache/v2/queue	0.010s
2020/03/12 18:57:32 Entry not found
2020/03/12 18:57:32 stored "testkey" in cache.
2020/03/12 18:57:32 empty request.
2020/03/12 18:57:32 stored "putKey" in cache.
2020/03/12 18:57:32 Entry not found
2020/03/12 18:57:32 empty request.
2020/03/12 18:57:32 invalidDeleteKey not found.
2020/03/12 18:57:32  not found.
2020/03/12 18:57:32 test read error
2020/03/12 18:57:32 entry is bigger than max shard size
PASS

I think it would be unnecessary to worry about the efficiency of uvarint encoding given it has been taken on many projects like leveldb etc.

siennathesane

I like this, but I have a couple concerns:

Let's not comment out test cases. I would like to find a way to not lose test coverage and ensure we're still covering functionality.
Some more docs are useful.
Memory ballooning. Since the buffer is now the max entry size by default, it'd be good to understand how much the memory footprint changes.

I like the concept of this PR, and I think it's a very specific performance increase that brings a lot of benefit, I think it just needs a bit of addressing. :)

queue/bytes_queue.go

neal-zhu · 2020-03-14T02:06:12Z

@mxplusb I would not like to lose test coverage too, sir.
But I think the problem is the old assertions about capacity rely on implementation detail too much, like this test.
I think we should come up with a new way to test Capacity maybe.

siennathesane

I think we should come up with a new way to test Capacity maybe.

I totally agree. I also don't want to accept an internally breaking change without ensuring there's comprehensive test coverage for it. @janisz will likely have a better idea of the right way to test it, so I'll let him provide feedback.

queue/bytes_queue_test.go

janisz

I think we can merge it once all assertions are back and prepare new release of bigcache since this may be a breaking change for some users.

queue/bytes_queue_test.go

queue/bytes_queue.go

neal-zhu · 2020-03-18T12:18:06Z

Why did the build fail? My pr should have nothing to do with data race I think.

siennathesane

Thanks for working with us on the feedback, it looks good to me!

neal-zhu · 2020-03-19T05:37:32Z

Great! I would also like to help you guys with this issue

Co-authored-by: zhumaohua <zhumaohua@megvii.com>

zhumaohua added 2 commits March 12, 2020 13:04

[opt] save extra 32bytes

e5081fc

[opt] encode length in varint format

410d43d

cristaloleg reviewed Mar 12, 2020

View reviewed changes

zhumaohua added 2 commits March 12, 2020 15:02

[opt] save PutUvarint calls

d8c0a6b

[opt] remove panic

d99b835

janisz reviewed Mar 12, 2020

View reviewed changes

[opt] Optimize getUvarintSize

b1014fc

siennathesane requested changes Mar 13, 2020

View reviewed changes

queue/bytes_queue.go Outdated Show resolved Hide resolved

queue/bytes_queue.go Outdated Show resolved Hide resolved

[opt] Add docstring

cc9dd25

neal-zhu requested a review from siennathesane March 14, 2020 02:10

siennathesane reviewed Mar 16, 2020

View reviewed changes

janisz reviewed Mar 17, 2020

View reviewed changes

queue/bytes_queue_test.go Outdated Show resolved Hide resolved

janisz reviewed Mar 18, 2020

View reviewed changes

[opt] Assertions back

cd24f19

janisz reviewed Mar 18, 2020

View reviewed changes

queue/bytes_queue_test.go Outdated Show resolved Hide resolved

janisz reviewed Mar 18, 2020

View reviewed changes

queue/bytes_queue_test.go Outdated Show resolved Hide resolved

janisz reviewed Mar 18, 2020

View reviewed changes

queue/bytes_queue.go Show resolved Hide resolved

neal-zhu added 2 commits March 18, 2020 20:29

[opt] Bring back full queue error

6899c7c

[opt] Add some comments

64cab2d

siennathesane approved these changes Mar 19, 2020

View reviewed changes

janisz approved these changes Mar 19, 2020

View reviewed changes

cristaloleg approved these changes Mar 19, 2020

View reviewed changes

cristaloleg merged commit 15aeb0b into allegro:master Mar 19, 2020

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

Optimize bytes_queue (allegro#207)

8fecd16

Co-authored-by: zhumaohua <zhumaohua@megvii.com>

Fabianexe mentioned this pull request Oct 30, 2020

Fix a bug that obscures the byte queue when it is full and is extended #251

Merged

janisz mentioned this pull request Nov 16, 2020

when invoke allocateAdditionalMemory() , if the free space is 128 bytes, bug occurs #253

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize bytes_queue #207

Optimize bytes_queue #207

neal-zhu commented Mar 12, 2020 •

edited

Loading

codecov-io commented Mar 12, 2020 •

edited

Loading

cristaloleg Mar 12, 2020

neal-zhu Mar 12, 2020 •

edited

Loading

cristaloleg Mar 12, 2020

neal-zhu Mar 12, 2020

janisz left a comment

neal-zhu commented Mar 12, 2020 •

edited

Loading

siennathesane left a comment

neal-zhu commented Mar 14, 2020 •

edited

Loading

siennathesane left a comment

janisz left a comment

neal-zhu commented Mar 18, 2020 •

edited

Loading

siennathesane left a comment

neal-zhu commented Mar 19, 2020 •

edited

Loading

Optimize bytes_queue #207

Optimize bytes_queue #207

Conversation

neal-zhu commented Mar 12, 2020 • edited Loading

codecov-io commented Mar 12, 2020 • edited Loading

Codecov Report

cristaloleg Mar 12, 2020

Choose a reason for hiding this comment

neal-zhu Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

cristaloleg Mar 12, 2020

Choose a reason for hiding this comment

neal-zhu Mar 12, 2020

Choose a reason for hiding this comment

janisz left a comment

Choose a reason for hiding this comment

neal-zhu commented Mar 12, 2020 • edited Loading

siennathesane left a comment

Choose a reason for hiding this comment

neal-zhu commented Mar 14, 2020 • edited Loading

siennathesane left a comment

Choose a reason for hiding this comment

janisz left a comment

Choose a reason for hiding this comment

neal-zhu commented Mar 18, 2020 • edited Loading

siennathesane left a comment

Choose a reason for hiding this comment

neal-zhu commented Mar 19, 2020 • edited Loading

neal-zhu commented Mar 12, 2020 •

edited

Loading

codecov-io commented Mar 12, 2020 •

edited

Loading

neal-zhu Mar 12, 2020 •

edited

Loading

neal-zhu commented Mar 12, 2020 •

edited

Loading

neal-zhu commented Mar 14, 2020 •

edited

Loading

neal-zhu commented Mar 18, 2020 •

edited

Loading

neal-zhu commented Mar 19, 2020 •

edited

Loading