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

AVM: Extract divideCeilUnsafely to help document opcode costing rationale #3867

Merged
merged 2 commits into from
Apr 11, 2022

Conversation

michaeldiamant
Copy link
Contributor

Extracts divideCeilUnsafely function to help document opcode costing rationale. Stems from discussion in #3857 (comment).

Analysis below shows divideCellUnsafely is inlined. Implies there's no overhead for adding the function. The logs show divideCell because I renamed the method after an initial pass.

~/dev/go-algorand/data/transactions/logic divide_ceil !1 ?2 ───────────────────────────────────────────────────────────────────── 04:06:17 PM
❯ go build -gcflags=-m=2 . 2>&1  | grep opcodes.go | grep 'divideCeil'
./opcodes.go:73:6: can inline divideCeil with cost 8 as: func(int, int) int { return (numerator + denominator - 1) / denominator }
./opcodes.go:77:6: can inline (*linearCost).compute with cost 43 as: method(*linearCost) func([]stackValue) int { cost := lc.baseCost; if lc.chunkCost != 0 && lc.chunkSize != 0 { cost += divideCeil(lc.chunkCost * len(stack[len(stack) - 1].Bytes), lc.chunkSize) }; return cost }
./opcodes.go:81:21: inlining call to divideCeil func(int, int) int { return (numerator + denominator - 1) / denominator }
./opcodes.go:140:28: inlining call to (*linearCost).compute method(*linearCost) func([]stackValue) int { cost := lc.baseCost; if lc.chunkCost != 0 && lc.chunkSize != 0 { cost += divideCeil(lc.chunkCost * len(stack[len(stack) - 1].Bytes), lc.chunkSize) }; return cost }
./opcodes.go:140:28: inlining call to divideCeil func(int, int) int { return (numerator + denominator - 1) / denominator }

As intended, benchmarking shows no meaningful change.

  • Benchmark run: go test ./data/transactions/logic -bench=BenchmarkUintMath
  • BenchmarkUintMath-diff.txt - old = master and new = the PR.

@michaeldiamant michaeldiamant requested a review from jannotti April 7, 2022 20:20
@michaeldiamant michaeldiamant changed the title Extract divideCeilUnsafely to help document opcode costing rationale AVM: Extract divideCeilUnsafely to help document opcode costing rationale Apr 7, 2022
Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

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

Yes, this is what I was thinking. I probably wouldn't say 'Unsafely' as that seems to be what division always is. But not something I feel strongly about.

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

Successfully merging this pull request may close these issues.

2 participants