Adjust formulas for calculating memory usage of arithmetic operations on big integers #1590
Conversation
Codecov Report
@@ Coverage Diff @@
## feature/memory-metering #1590 +/- ##
===========================================================
+ Coverage 76.40% 76.49% +0.09%
===========================================================
Files 291 291
Lines 58665 58782 +117
===========================================================
+ Hits 44822 44967 +145
+ Misses 12265 12231 -34
- Partials 1578 1584 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:feature/memory-metering commit 2b47860 Results
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
FYI, I am back today and answered the questions here. |
There was a problem hiding this comment.
I left a late review.
- The code is overall correct except some small errors.
- I also noticed some edge cases to tighten the upper bound were omitted (like zero big integers), maybe to simplify the code?
- If Cadence is accepting big integer shift values, it might make sense to update the API in the future to only accept 32 or 64-bits shift values.
| func NewPlusBigIntMemoryUsage(a, b *big.Int) MemoryUsage { | ||
| // max(|a|, |b|) + 5 |
There was a problem hiding this comment.
I see that the cases |a|==|b|==0 were skipped. I guess that's because it makes the metering more complex?
The downside is that the metering becomes more "loose", but that's okay if you are fine with that.
| } else { | ||
| recursionCost := int(unsafe.Sizeof(uintptr(0))) + | ||
| 9*bWordLength + | ||
| int(math.Floor(float64(aWordLength)/float64(bWordLength))) + 12 |
There was a problem hiding this comment.
On the document, I wrote Floor because that's the math specific operation. In software languages, integer division implicitly returns that floor.
We could replace int(math.Floor(float64(aWordLength)/float64(bWordLength))) by aWordLength/bWordLength.
| var resultWordLength int | ||
| if a.Cmp(b) < 0 || b.Cmp(bigOne) == 0 { | ||
| resultWordLength = aWordLength + 4 | ||
| } else if b.Cmp(bigOneHundred) < 0 { |
There was a problem hiding this comment.
This should check |b| < 100 while it the code does b<100.
Same for the check |b|==1, it shouldn't be b==1.
| ) | ||
| } | ||
|
|
||
| var invalidLeftShift = errors.New("invalid left shift of non-Int64") | ||
|
|
||
| func NewBitwiseLeftShiftBigIntMemoryUsage(a, b *big.Int) MemoryUsage { |
There was a problem hiding this comment.
I don't think the shift value should be a big integer, a uint is more than enough.
Then in the metering b/word_size would avoid using a big integer division ( I guess the metering itself should be very light and should not allocate memory)
| resultWordLength = aWordLength + 4 | ||
| } else { | ||
| // TODO: meter the allocation of the metering itself | ||
| shiftByteLengthBig := new(big.Int).Div(b, bigIntWordSizeAsBig) |
There was a problem hiding this comment.
This the operation I mentioned in the last comment.
| // TODO: https://github.com/dapperlabs/cadence-private-issues/issues/32 | ||
| BigIntByteLength(a) + | ||
| BigIntByteLength(b), | ||
| resultWordLength * bigIntWordSize, | ||
| ) | ||
| } | ||
|
|
||
| func NewBitwiseRightShiftBigIntMemoryUsage(a, b *big.Int) MemoryUsage { |
There was a problem hiding this comment.
Same here for the shift value that should be uint
Closes https://github.com/dapperlabs/cadence-private-issues/issues/32
Based on https://www.notion.so/dapperlabs/Metering-arithmetic-operations-7acc852f89834e78911f8f42c6bb842d.
Not complete, I left some questions in the document.
masterbranchFiles changedin the Github PR explorer