Use uint64 intead of uint32 #236

janisz · 2020-08-21T18:55:24Z

There are posibility we run into a problem of int32 overflow.
To prevent this let's use uint64 everywhere.

Line 138 in 21e5ca5

s.hashmap[hashedKey] = uint32(index)

Fixes: #148

siennathesane · 2020-08-21T19:42:04Z

I like this, but we should document that this change breaks 32-bit compatibility (assuming it existed already), and this should likely be a new version.

siennathesane · 2020-10-05T18:53:53Z

@cristaloleg do you want this to be a part of 3.0 since it breaks 32-bit compatibility or just cut a new release as normal?

cristaloleg · 2020-10-05T19:43:06Z

Well, breaking 32bit compatibility in patch/minor version is a bad thing. Better to postpone till 3.0 :(

siennathesane · 2020-10-05T20:01:07Z

@janisz if you can rebase against the versions/v3-prep branch, then edit the PR to point to that branch, we can get this merged in.

janisz · 2020-10-07T18:50:19Z

@mxplusb Done

Issues reported in allegro#246

janisz · 2020-10-08T20:26:33Z

Interesting, race checker adds more memory

➜  bigcache git:(fix_148) ✗ go test
2020/10/08 22:24:22 Allocated new queue in 492ns; Capacity: 88 
2020/10/08 22:24:22 Allocated new queue in 5.826µs; Capacity: 256046 
2020/10/08 22:24:22 Allocated new queue in 122.952µs; Capacity: 512092 
2020/10/08 22:24:22 Allocated new queue in 318.934µs; Capacity: 1024184 
2020/10/08 22:24:23 Allocated new queue in 267.441µs; Capacity: 585000 
2020/10/08 22:24:23 Allocated new queue in 294.733µs; Capacity: 1170000 
PASS
ok  	github.com/allegro/bigcache/v3	14.906s
➜  bigcache git:(fix_148) ✗ go test -race
signal: killed
FAIL	github.com/allegro/bigcache/v3	19.155s

➜  bigcache git:(fix_148) ✗ dmesg
[44698.559338] oom-kill:constraint=CONSTRAINT_NONE,nodemask=(null),cpuset=/,mems_allowed=0,global_oom,task_memcg=/user.slice,task=bigcache.test,pid=26385,uid=1000
[44698.559368] Out of memory: Killed process 26385 (bigcache.test) total-vm:31216124kB, anon-rss:19200356kB, file-rss:0kB, shmem-rss:3452kB, UID:1000 pgtables:39016kB oom_score_adj:0
[44698.973775] oom_reaper: reaped process 26385 (bigcache.test), now anon-rss:0kB, file-rss:0kB, shmem-rss:0kB

➜  bigcache git:(fix_148) ✗ go version
go version go1.15.2 linux/amd64

janisz · 2020-10-08T20:37:28Z

➜  bigcache git:(fix_148) ✗ go test -race -c -o test_race github.com/allegro/bigcache/v3 
➜  bigcache git:(fix_148) ✗ /usr/bin/time -v ./test_race                                 
Command terminated by signal 9
	Command being timed: "./test_race"
	User time (seconds): 19.79
	System time (seconds): 9.03
	Percent of CPU this job got: 151%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 0:18.97
	Average shared text size (kbytes): 0
	Average unshared data size (kbytes): 0
	Average stack size (kbytes): 0
	Average total size (kbytes): 0
	Maximum resident set size (kbytes): 18998948
	Average resident set size (kbytes): 0
	Major (requiring I/O) page faults: 130
	Minor (reclaiming a frame) page faults: 8179180
	Voluntary context switches: 2791
	Involuntary context switches: 558
	Swaps: 0
	File system inputs: 27864
	File system outputs: 0
	Socket messages sent: 0
	Socket messages received: 0
	Signals delivered: 0
	Page size (bytes): 4096
	Exit status: 0
➜  bigcache git:(fix_148) ✗ go test -c -o test github.com/allegro/bigcache/v3  
➜  bigcache git:(fix_148) ✗ /usr/bin/time -v ./test                           
PASS
	Command being timed: "./test"
	User time (seconds): 19.65
	System time (seconds): 3.31
	Percent of CPU this job got: 146%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 0:15.71
	Average shared text size (kbytes): 0
	Average unshared data size (kbytes): 0
	Average stack size (kbytes): 0
	Average total size (kbytes): 0
	Maximum resident set size (kbytes): 13417992
	Average resident set size (kbytes): 0
	Major (requiring I/O) page faults: 0
	Minor (reclaiming a frame) page faults: 1020072
	Voluntary context switches: 324544
	Involuntary context switches: 640
	Swaps: 0
	File system inputs: 0
	File system outputs: 0
	Socket messages sent: 0
	Socket messages received: 0
	Signals delivered: 0
	Page size (bytes): 4096
	Exit status: 0

codecov-io · 2020-10-08T22:21:36Z

Codecov Report

❗ No coverage uploaded for pull request base (master@9949a06). Click here to learn what that means.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master     #236   +/-   ##
=========================================
  Coverage          ?   87.18%           
=========================================
  Files             ?       15           
  Lines             ?      640           
  Branches          ?        0           
=========================================
  Hits              ?      558           
  Misses            ?       69           
  Partials          ?       13

Impacted Files	Coverage Δ
server/server.go	`29.03% <ø> (ø)`
queue/bytes_queue.go	`92.59% <100.00%> (ø)`
shard.go	`84.93% <100.00%> (ø)`
iterator.go	`88.67% <0.00%> (ø)`
bigcache.go	`98.79% <0.00%> (ø)`
fnv.go	`100.00% <0.00%> (ø)`
utils.go	`62.50% <0.00%> (ø)`
bytes.go	`100.00% <0.00%> (ø)`
logger.go	`100.00% <0.00%> (ø)`
... and 8 more

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 9949a06...6a48d10. Read the comment docs.

janisz · 2020-10-08T22:26:50Z

Well, breaking 32bit compatibility in patch/minor version is a bad thing. Better to postpone till 3.0 :(

I'm not sure if this breaks compatibility. API is the same so on 32bits it will work as before

Co-authored-by: Oleg Kovalov <[email protected]>

Signed-off-by: Mike Lloyd <[email protected]>

There are posibility we run into a problem of int32 overflow. To prevent this let's use uint64 everywhere. https://github.com/allegro/bigcache/blob/21e5ca5c3d539f94e8dc563350acd97c5400154f/shard.go#L138 Fixes: allegro#148

janisz · 2020-10-12T16:51:01Z

I removed 1.13 since it's failing. I need to debug why

janisz · 2020-10-12T17:03:25Z

It's passing on my machine?

➜  bigcache git:(fix_148) ✗ go test -short -race            
2020/10/12 19:00:06 Allocated new queue in 51.32µs; Capacity: 256046 
2020/10/12 19:00:06 Allocated new queue in 1.668359ms; Capacity: 512092 
2020/10/12 19:00:06 Allocated new queue in 1.031µs; Capacity: 88 
2020/10/12 19:00:06 Allocated new queue in 3.652714ms; Capacity: 1024184 
2020/10/12 19:00:06 Allocated new queue in 2.43308ms; Capacity: 585000 
2020/10/12 19:00:06 Allocated new queue in 5.981636ms; Capacity: 1170000 
PASS
ok  	github.com/allegro/bigcache/v3	42.315s
➜  bigcache git:(fix_148) ✗ go version
go version go1.13.15 linux/amd64

janisz · 2020-10-13T10:43:39Z

@cristaloleg @mxplusb PTAL

cristaloleg · 2020-10-14T07:03:26Z

.github/workflows/build.yml

@@ -42,7 +42,7 @@ jobs:
        run: |
          go test -race -count=1 -coverprofile=queue.coverprofile ./queue
          go test -race -count=1 -coverprofile=server.coverprofile  ./server
-          go test -race -count=1 -coverprofile=main.coverprofile
+          go test -race -count=1 -coverprofile=main.coverprofile -short


wait a sec, why short?

Because new test eats too much memory
https://github.com/allegro/bigcache/pull/236/files#diff-013414c814f8b4b2df17143a678bcc6726758e92f0be44d01df40e3dca9ad967R1072

siennathesane · 2020-10-15T15:03:25Z

@cristaloleg It looks like the 1.13.x build requirement is blocking the merge, can you remove it?

* Add Benchmark for append * Optimize Append and halve byte copies * Optimize Append by reducing allocs * Optimize Append by reducing allocs * Reduces allocs from test construct Co-authored-by: Fabian Gärtner <[email protected]>

janisz · 2020-10-31T18:56:14Z

@cristaloleg PTAL

siennathesane · 2020-11-04T01:20:36Z

I'm merging this into the v3 base so we can close out the work item so it's in 3.0.

* Fix iterator styling issues (#247) Issues reported in #246 * fix #160 (#246) * Update to latest golang (#248) Co-authored-by: Oleg Kovalov <[email protected]> * inital prep for v3. Signed-off-by: Mike Lloyd <[email protected]> * Use uint64 intead of uint32 There are posibility we run into a problem of int32 overflow. To prevent this let's use uint64 everywhere. https://github.com/allegro/bigcache/blob/21e5ca5c3d539f94e8dc563350acd97c5400154f/shard.go#L138 Fixes: #148 * Fix CI * Do not run on 1.13 * Do not run long test * Optimze append (#249) * Add Benchmark for append * Optimize Append and halve byte copies * Optimize Append by reducing allocs * Optimize Append by reducing allocs * Reduces allocs from test construct Co-authored-by: Fabian Gärtner <[email protected]> Co-authored-by: S@P <[email protected]> Co-authored-by: Oleg Kovalov <[email protected]> Co-authored-by: Mike Lloyd <[email protected]> Co-authored-by: Fabianexe <[email protected]> Co-authored-by: Fabian Gärtner <[email protected]>

janisz force-pushed the fix_148 branch from 69e0c58 to 8ca2ca3 Compare August 21, 2020 18:58

siennathesane added this to the 3.0.0 milestone Aug 31, 2020

janisz force-pushed the fix_148 branch from 1c7d9de to 35330c5 Compare October 7, 2020 18:50

janisz force-pushed the fix_148 branch 2 times, most recently from 6ec47a5 to e861020 Compare October 7, 2020 19:05

janisz and others added 2 commits October 8, 2020 09:19

Fix iterator styling issues (allegro#247)

0afcdc4

Issues reported in allegro#246

fix allegro#160 (allegro#246)

d615854

janisz force-pushed the fix_148 branch from 91f575f to 46cecc5 Compare October 8, 2020 18:38

janisz force-pushed the fix_148 branch from 0233928 to 8d31032 Compare October 8, 2020 21:56

janisz and others added 3 commits October 9, 2020 06:32

Update to latest golang (allegro#248)

4b1f942

Co-authored-by: Oleg Kovalov <[email protected]>

inital prep for v3.

0ca4bb2

Signed-off-by: Mike Lloyd <[email protected]>

Use uint64 intead of uint32

c4822a0

There are posibility we run into a problem of int32 overflow. To prevent this let's use uint64 everywhere. https://github.com/allegro/bigcache/blob/21e5ca5c3d539f94e8dc563350acd97c5400154f/shard.go#L138 Fixes: allegro#148

janisz force-pushed the fix_148 branch from c42d028 to 13fbedd Compare October 12, 2020 15:31

Fix CI

ee9ee17

janisz force-pushed the fix_148 branch from 13fbedd to ee9ee17 Compare October 12, 2020 16:03

janisz added 2 commits October 12, 2020 18:08

Do not run on 1.13

ed5c029

Do not run long test

ffe7004

cristaloleg previously approved these changes Oct 14, 2020

View reviewed changes

Optimze append (allegro#249)

9949a06

* Add Benchmark for append * Optimize Append and halve byte copies * Optimize Append by reducing allocs * Optimize Append by reducing allocs * Reduces allocs from test construct Co-authored-by: Fabian Gärtner <[email protected]>

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

Merge branch 'master' into fix_148

6a48d10

janisz dismissed cristaloleg’s stale review via 6a48d10 October 31, 2020 13:54

janisz force-pushed the fix_148 branch from 89c4c0f to 6a48d10 Compare October 31, 2020 13:54

janisz requested a review from cristaloleg October 31, 2020 18:56

siennathesane changed the base branch from master to versions/v3-prep November 4, 2020 01:19

siennathesane merged commit 64eb605 into allegro:versions/v3-prep Nov 4, 2020

ppai-plivo mentioned this pull request Jan 28, 2021

queue: Push() panics in v3.0.0 #263

Open

janisz mentioned this pull request Sep 8, 2021

panic out of range in bytes queue #148

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use uint64 intead of uint32 #236

Use uint64 intead of uint32 #236

janisz commented Aug 21, 2020

siennathesane commented Aug 21, 2020

siennathesane commented Oct 5, 2020

cristaloleg commented Oct 5, 2020

siennathesane commented Oct 5, 2020

janisz commented Oct 7, 2020

janisz commented Oct 8, 2020 •

edited

Loading

janisz commented Oct 8, 2020

codecov-io commented Oct 8, 2020 •

edited

Loading

janisz commented Oct 8, 2020

janisz commented Oct 12, 2020

janisz commented Oct 12, 2020

janisz commented Oct 13, 2020

cristaloleg Oct 14, 2020

janisz Oct 14, 2020

siennathesane commented Oct 15, 2020

janisz commented Oct 31, 2020

siennathesane commented Nov 4, 2020

Use uint64 intead of uint32 #236

Use uint64 intead of uint32 #236

Conversation

janisz commented Aug 21, 2020

siennathesane commented Aug 21, 2020

siennathesane commented Oct 5, 2020

cristaloleg commented Oct 5, 2020

siennathesane commented Oct 5, 2020

janisz commented Oct 7, 2020

janisz commented Oct 8, 2020 • edited Loading

janisz commented Oct 8, 2020

codecov-io commented Oct 8, 2020 • edited Loading

Codecov Report

janisz commented Oct 8, 2020

janisz commented Oct 12, 2020

janisz commented Oct 12, 2020

janisz commented Oct 13, 2020

cristaloleg Oct 14, 2020

Choose a reason for hiding this comment

janisz Oct 14, 2020

Choose a reason for hiding this comment

siennathesane commented Oct 15, 2020

janisz commented Oct 31, 2020

siennathesane commented Nov 4, 2020

janisz commented Oct 8, 2020 •

edited

Loading

codecov-io commented Oct 8, 2020 •

edited

Loading