-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[DO-NOT-MERGE] .ef optimization experiments #12907
base: main
Are you sure you want to change the base?
Conversation
note: code quality is "hacky", pls do not consider for review yet, only for evaluation of the overall solution + validating the strategy |
now that the prototype 1 is out, let me rebase it to main as it is 20 days outdated... |
3ea3702
to
9e5031a
Compare
9e7c1b2
to
ae6c0bf
Compare
As part of #12907 I'll have other iterator-like sequence implementations. It makes sense to generalize the ErrEliasFanoIterExhausted and move it to a common package and reuse it, rather than making a bunch of IteratorExhausted-like for each implementation.
ae6c0bf
to
6d7f604
Compare
Current status:
Next steps:
|
@wmitsuda FYI: maybe next commands may help to test files |
928dc9d
to
b0c3db9
Compare
Current status:
so, we can conclude data is correct + size reduction is the expected one so far. |
from now on, I think we are ready to advance one more step towards making it stable:
|
f5dfa8c
to
b6db9b9
Compare
} | ||
|
||
// simple encoding | ||
if data[0]&0b11110000 == 0b10000000 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use const
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forgot to use the existing SimpleEncoding const... will also add one for the comparison mask
// from raw data | ||
func Count(baseNum uint64, data []byte) uint64 { | ||
// plain elias fano (legacy) | ||
if data[0]&0b10000000 == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thats Simple Encoding constant value, probably zero bit could be just compared to 0b0 const
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, because if the 8th bit is 0, meaning it is legacy elias fano encoding type, I'm using that entire byte as part of the following elias fano struct, so in theory the remaining 7 bits of data[0] will be used to decode elias fano.
anyway, will add more consts to clarify that comparison.
} | ||
|
||
// simple encoding | ||
if raw[0]&0b11110000 == 0b10000000 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whats that mask 0xf0? using const is better, also seems can use just 2 bits instead of 8, have something to encode up to 2^6? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because I'm reserving the range [0b10000000, 0b100001111] to SimpleEncoding. I'm using the it to encode at the same time the type (SimpleEncoding) + the number of elements (up to 16) in the same byte and save space.
So, the masking + comparison is saying: ignore the less significant 4 bits, if it is 0b1000????, then it is SimpleEncoding.
yeah, will add consts, forgot it :)
I didn't understand the part about 2^6?
it.Seek(uint64(v)) | ||
} | ||
return it | ||
} else if s.currentEnc == PlainEliasFano || s.currentEnc == RebasedEliasFano { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw since constants are growing could just do s.currentEnc < threshold. So maybe reorder them as PlainEF, RebasedEF, and then SimpleEnc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, problem here is that the consts are at the same time the type itself and the bit fragments that are used when encoding them, i.e., they are not 1, 2 and 3, but 0, 128 and 144.
I think I could do it, but I think the code now is clearer to read and understand?
Also, if we are going to merge it now, maybe when can even remove the plain elias-fano choice since it would not be used anymore in this part of code and have one less comparison.
No description provided.