engine: page defragmentation #202
Conversation
|
Hey thanks for the PR! Im not sure about the addition of use of the PR but we dont allow panics in the code - please ensure the CI passes :D |
|
Join can check out of one of the pinned issues to join our slack too! |
Hey! That's was fast - you replied even before the CI finished :) Thanks, fixed |
sorry, not sure that I got it. I joined the slack, btw |
|
Haha yes our slack bot notified me. @TimSatke is actually the code owner for this part, it'll be reviewed by him :D |
There was a problem hiding this comment.
Please have a look at https://github.com/tomarrell/lbadd/blob/master/CODING_GUIDELINES.md .
Code looks good. Major flaws were probably due to lack of knowledge of features, such as RawData instead of copying the page data yourself.
- We use the
assertpackage of https://github.com/stretchr/testify for testing and benchmarks. - Use of random in tests must always be explicitly seeded (probably gonna add that to the coding guidelines).
- Ordering:
const -> var -> struct -> exported methods -> unexported methods -> exported functions -> unexported functions
| @@ -0,0 +1,63 @@ | |||
| package page | |||
There was a problem hiding this comment.
Please read the contribution and coding guidelines.
| pageSize = int(math.Round(2 * float64(Size) / 3)) // two-thirds of the max size | ||
| ) | ||
|
|
||
| func generateRecordCell(b *testing.B) RecordCell { |
There was a problem hiding this comment.
Ordering: exported before unexported
| copy(bytes, data) | ||
| page := &Page{ // have to recreate a page with initial data | ||
| data: bytes, | ||
| } |
There was a problem hiding this comment.
This should probably not be part of the benchmark, exclude it with b.StopTimer() and b.StartTimer() respectively
| keySize := 1 + rand.Intn(maxKeySize-1) | ||
| recordSize := 1 + rand.Intn(maxRecordSize-1) |
There was a problem hiding this comment.
Please use an explicitely seeded random source, to be sure that the test is always reproducible, regardless of someone else seeding the whole rand package or not.
|
|
||
| func generatePage(b *testing.B) *Page { | ||
| b.Helper() | ||
| p, err := load(make([]byte, pageSize)) |
There was a problem hiding this comment.
Why only use a page with 2 thirds of the total size? Is that benchmark still representative if the page is smaller than 16K?
|
|
||
| for i := 0; i < b.N; i++ { | ||
| copy(bytes, data) | ||
| page := &Page{ // have to recreate a page with initial data |
There was a problem hiding this comment.
Use Load rather than this. This does work, however, if the page implementation changes, this probably doesn't work any more (e.g. adding locks or cell caches as maps etc)
| if err != nil { | ||
| b.Fatal(err) | ||
| } |
There was a problem hiding this comment.
We tend to use stretchr/testify for tests.
You could easily rewrite this as
assert := assert.New(b)
...
_, err := rand.Read(key)
assert.NoError(err)
...
And you would get rid of a lot of err checks like this.
| } | ||
|
|
||
| func generatePage(b *testing.B) *Page { | ||
| b.Helper() |
There was a problem hiding this comment.
Why is this a helper, but generateCell isn't?
| record := generateRecordCell(b) | ||
| for err = p.StoreRecordCell(record); err != ErrPageFull; err = p.StoreRecordCell(record) { | ||
| record = generateRecordCell(b) | ||
| } |
There was a problem hiding this comment.
| record := generateRecordCell(b) | |
| for err = p.StoreRecordCell(record); err != ErrPageFull; err = p.StoreRecordCell(record) { | |
| record = generateRecordCell(b) | |
| } | |
| for { | |
| rec := generateRecordCell(b) | |
| if err := p.StoreRecordCell(rec); err != nil { | |
| assert.Equal(ErrPageFull, err) | |
| break | |
| } | |
| } |
| data := generatePage(b).data | ||
| bytes := make([]byte, len(data)) | ||
| b.ResetTimer() | ||
| b.ReportAllocs() | ||
|
|
||
| for i := 0; i < b.N; i++ { | ||
| copy(bytes, data) |
There was a problem hiding this comment.
page.RawData() does exactly that, return a copy of the page data. That way, you don't need to access unexported fields.
|
Closing this as defragmentation will disappear. Page is defragmented upon delete, not when someone calls it. |
Implement a benchmark for page defragmentation operation (part of #183)