Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The size encoded in a entry header contains the header it self. #266

Merged
merged 5 commits into from
Nov 18, 2021
Merged

The size encoded in a entry header contains the header it self. #266

merged 5 commits into from
Nov 18, 2021

Conversation

Fabianexe
Copy link
Contributor

As #253 has pointed out, it is impossible to create an entry of arbitrary size in the byte queue. Basically, the problem is that at the points where the header gets larger, one number is skipped. This can lead to errors in the new allocation of a byte queue.

To solve this, the header size is included in the number that is encoded. This allows creating entries with arbitrary dimensions. For example, we have 127 bytes (1 header, 126 data), 128bytes (2 header, 126 data), and 129 bytes(2 header, 127 data).

As a benefit, we can reduce some calculations, and the code gets more straightforward.
The only downside is that the maximum entry size reduces from 4294967296 to 4294967291 bytes. In my belief, nothing too dramatic that gets in v3 with uint64, even a more minor problem.

This make it possible to create entries of arbitrary size.
@Fabianexe
Copy link
Contributor Author

@KaixuanYu @janisz

@user3415653
Copy link

Any news here? Sometimes we get this error, so a fix would be nice.

@janisz
Copy link
Collaborator

janisz commented Aug 18, 2021

@Fabianexe Could you rebase?
@cristaloleg Could you take a look?

Copy link

@ekkoxx ekkoxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix

@panmari
Copy link
Contributor

panmari commented Oct 30, 2021

Anything else needed here? I'd love to see this merged.

@panmari
Copy link
Contributor

panmari commented Nov 14, 2021

On HEAD, I'm still seeing the error panic: runtime error: index out of range [7] with length 0 with the trace

encoding/binary.littleEndian.Uint64(...)
encoding/binary/binary.go:77
bigcache/bigcache.readTimestampFromEntry(...)
bigcache/encoding.go:58
bigcache/bigcache.(*cacheShard).onEvict(0xc008d6e008?, {0xc27fe50e69?, 0x1c6b0668dbacaecc?, 0x0?}, 0x5593c1f7d065?, 0xc031bc5888?)
bigcache/shard.go:271 +0x65
bigcache/bigcache.(*cacheShard).set(0xc008d6e000, {0xc0510a08a0, 0x25}, 0x1c6b0668dbacaecc, {0x0, 0x0, 0x25?})
bigcache/shard.go:134 +0x197
bigcache/bigcache.(*BigCache).Set(0xc002a149c0, {0xc0510a08a0, 0x25}, {0x0, 0x0, 0x0})

What's blocking here? Do you want me to do some more evaluation before merging?

@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2021

Codecov Report

Merging #266 (866d8e7) into master (0c76355) will increase coverage by 0.77%.
The diff coverage is 76.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #266      +/-   ##
==========================================
+ Coverage   88.45%   89.22%   +0.77%     
==========================================
  Files          15       15              
  Lines         762      761       -1     
==========================================
+ Hits          674      679       +5     
+ Misses         73       69       -4     
+ Partials       15       13       -2     
Impacted Files Coverage Δ
queue/bytes_queue.go 92.00% <76.00%> (+1.52%) ⬆️
shard.go 88.49% <0.00%> (+1.58%) ⬆️

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 0c76355...866d8e7. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants