fix(inclusion): fix integer overflow+inefficient Round*PowerOfTwo #118
Conversation
odeke-em
requested review from
rootulp,
rach-id,
cristaloleg and
vgonkivs
and removed request for
a team
odeke-em
force-pushed
the
inclusion-fix-RoundDownPowerOfTwo+RoundUpPowerOfTwo
branch
4 times, most recently
from
9f9516b
to
d838e40
Compare
…und*PowerOfTwo This code fixes an integer flow using finer bit twiddling and even makes it much more efficient to always run deterministically in O(1) time essentially and not O(k) where k=log2(input) due to the prior k iterations that then caused the overflow when result became > maxInt. While here also added some benchmarks and more test cases. Fixes celestiaorg#117
odeke-em
force-pushed
the
inclusion-fix-RoundDownPowerOfTwo+RoundUpPowerOfTwo
branch
from
d838e40
to
8d14642
Compare
| if input&(input-1) == 0 { // It is already a power of 2 | ||
| return input | ||
| } | ||
| var powUp I = 1 << bits.Len64(uint64(input)) |
There was a problem hiding this comment.
[Optional]
I would add a comment here explaining why this works
| want: 2, | ||
| }, | ||
| { | ||
| shareCount: (defaultSubtreeRootThreshold * 2) + 1, |
There was a problem hiding this comment.
[Optional]
I would add something like:
| shareCount: (defaultSubtreeRootThreshold * 2) + 1, | |
| shareCount: (defaultSubtreeRootThreshold * 2) + 1, // 129 |
to avoid needing to calculate the numbers when reading the tests
celestia-bot
requested review from
a team and
cmwaters
and removed request for
a team
| if input&(input-1) == 0 { // It is already a power of 2 | ||
| return input | ||
| } |
There was a problem hiding this comment.
[nit] turn the comment into self-documenting code
| if input&(input-1) == 0 { // It is already a power of 2 | |
| return input | |
| } | |
| if isPowerOfTwo(input) { | |
| return input | |
| } |
by extracting a function
func RoundUpPowerOfTwo[I constraints.Integer](input I) bool {
return input&(input-1) == 0
}| if powUp < input { | ||
| // An overflow occurred due to a very large size | ||
| // of input and we should return a positive power of 2. | ||
| powUp = 1 | ||
| } |
There was a problem hiding this comment.
[blocking] this is unexpected behavior. I would rather this function return an error here than quietly return 1.
| if input&(input-1) == 0 { // It is already a power of 2 | ||
| return input, nil | ||
| } |
There was a problem hiding this comment.
same nit: extract a function and delete the comment
| {input: math.MaxInt32 + 1, want: 1 << 31}, | ||
| {input: math.MaxInt32, want: 1 << 31}, | ||
| {input: math.MaxInt >> 1, want: 1 << 62}, | ||
| {input: math.MaxInt, want: 1}, |
There was a problem hiding this comment.
[blocking] this seems like unexpected behavior. I think we should return an error in this case
| type roundUpTestCase struct { | ||
| input int | ||
| want int | ||
| } | ||
|
|
||
| var roundUpTestCases = []roundUpTestCase{ | ||
| {input: -1, want: 1}, | ||
| {input: 0, want: 1}, | ||
| {input: 1, want: 1}, | ||
| {input: 2, want: 2}, | ||
| {input: 4, want: 4}, | ||
| {input: 5, want: 8}, | ||
| {input: 8, want: 8}, | ||
| {input: 11, want: 16}, | ||
| {input: 511, want: 512}, | ||
| {input: math.MaxInt32 - 1, want: 1 << 31}, | ||
| {input: math.MaxInt32 + 1, want: 1 << 31}, | ||
| {input: math.MaxInt32, want: 1 << 31}, | ||
| {input: math.MaxInt >> 1, want: 1 << 62}, | ||
| {input: math.MaxInt, want: 1}, | ||
| } |
There was a problem hiding this comment.
[blocking] these were previously defined inside the function TestRoundUpPowerOfTwo. Can you please move them back there so that their scope is confined to that function
| type subtreeWidthTestCase struct { | ||
| shareCount int | ||
| want int | ||
| } | ||
|
|
||
| var subtreeWidthTestCases = []subtreeWidthTestCase{ | ||
| { | ||
| shareCount: 0, | ||
| want: 1, | ||
| }, | ||
| { | ||
| shareCount: 1, | ||
| want: 1, | ||
| }, | ||
| { | ||
| shareCount: 2, | ||
| want: 1, | ||
| }, | ||
| { | ||
| shareCount: defaultSubtreeRootThreshold, | ||
| want: 1, | ||
| }, | ||
| { | ||
| shareCount: defaultSubtreeRootThreshold + 1, | ||
| want: 2, | ||
| }, | ||
| { | ||
| shareCount: defaultSubtreeRootThreshold - 1, | ||
| want: 1, | ||
| }, | ||
| { | ||
| shareCount: defaultSubtreeRootThreshold * 2, | ||
| want: 2, | ||
| }, | ||
| { | ||
| shareCount: (defaultSubtreeRootThreshold * 2) + 1, | ||
| want: 4, | ||
| }, | ||
| { | ||
| shareCount: (defaultSubtreeRootThreshold * 3) - 1, | ||
| want: 4, | ||
| }, | ||
| { | ||
| shareCount: (defaultSubtreeRootThreshold * 4), | ||
| want: 4, | ||
| }, | ||
| { | ||
| shareCount: (defaultSubtreeRootThreshold * 5), | ||
| want: 8, | ||
| }, | ||
| { | ||
| shareCount: (defaultSubtreeRootThreshold * defaultMaxSquareSize) - 1, | ||
| want: 128, | ||
| }, | ||
| } |
There was a problem hiding this comment.
[blocking] please define subtreeWidthTestCase and subtreeWidthTestCases inside the function TestSubTreeWidth to confine their scope.
| type roundDownTestCase struct { | ||
| input int | ||
| want int | ||
| } | ||
|
|
||
| var roundDownTestCases = []roundDownTestCase{ | ||
| {input: 1, want: 1}, | ||
| {input: 2, want: 2}, | ||
| {input: 4, want: 4}, | ||
| {input: 5, want: 4}, | ||
| {input: 8, want: 8}, | ||
| {input: 11, want: 8}, | ||
| {input: 511, want: 256}, | ||
| {input: math.MaxInt32 - 1, want: 1 << 30}, | ||
| {input: math.MaxInt32, want: 1 << 30}, | ||
| {input: math.MaxInt32 + 1, want: 1 << 31}, | ||
| {input: math.MaxInt, want: 1 << 62}, | ||
| } |
There was a problem hiding this comment.
Please define these inside TestRoundDownPowerOfTwo
| type testCase struct { | ||
| input int | ||
| want int | ||
| var sink any |
There was a problem hiding this comment.
[question] should this be defined inside each Benchmark* test so that one benchmark does not impact another?
This code fixes an integer flow using finer bit twiddling and even makes it much more efficient to always run deterministically in O(1) time essentially and not O(k) where k=log2(input) due to the prior k iterations that then caused the overflow when result became > maxInt.
While here also added some benchmarks and more test cases.
Fixes #117