Repository navigation
Formatting an i8 takes a long time after nightly-2023-06-20 upgrade #2576
Description
Activity
- added[C] BugThis is a bug. Something isn't working.This is a bug. Something isn't working.[E] PerformanceTrack performance improvement (Time / Memory / CPU)Track performance improvement (Time / Memory / CPU)
on Jun 28, 2023 Comparing Kani before and after the upgrade. I noticed the following:
- There was no significant changes to the reachability analysis. The number of intrinsics decreased by 1/3 due to the new lowering logic.
MIR Reachability Results: Before Upgrade
====== Reachability Analysis Result ======= Total # items: 693 Total # statements: 5788 Total # expressions: 2832 Reachable Items: - function: 691 - static item: 2 Statements: - Assign: 2832 - Call: 1343 - Return: 669 - SwitchInt: 399 - Goto: 302 - Assert: 128 - Unreachable: 56 - Intrinsic: 32 - Drop: 27 Expressions: - Use: 1083 - Aggregate: 375 - BinaryOp: 309 - Cast: 304 - Ref: 259 - UnaryOp: 117 - Discriminant: 105 - CheckedBinaryOp: 78 - CopyForDeref: 74 - Len: 67 - AddressOf: 46 - NullaryOp: 10 - Repeat: 5 -------------------------------------------MIR Reachability Results: After Upgrade
====== Reachability Analysis Result ======= Total # items: 624 Total # statements: 5268 Total # expressions: 2662 Reachable Items: - function: 622 - static item: 2 Statements: - Assign: 2662 - Call: 1142 - Return: 600 - SwitchInt: 376 - Goto: 278 - Assert: 118 - Unreachable: 56 - Drop: 25 - Intrinsic: 11 Expressions: - Use: 1027 - Aggregate: 347 - BinaryOp: 309 - Cast: 283 - Ref: 245 - Discriminant: 103 - UnaryOp: 92 - CheckedBinaryOp: 76 - CopyForDeref: 74 - Len: 67 - AddressOf: 25 - NullaryOp: 9 - Repeat: 5 --------------------------------------------
The size of the goto model also didn't change much. With --show-goto-functions, the model went from 24367 to 24168 lines and the number of loops decreased slightly, from 32 loops to 29 loops after, comparing before and after upgrade.
-
The Symbolic Engine seems to have explored a much higher number of program expressions, which generated a much bigger VCC and consequently a much bigger formula.
CBMC Stats: Before Upgrade
Runtime Symex: 1.4556s size of program expression: 40831 steps slicing removed 27725 assignments Generated 2502 VCC(s), 1127 remaining after simplification Runtime Postprocess Equation: 0.036371s Passing problem to propositional reduction converting SSA Runtime Convert SSA: 0.233992s Running propositional reduction Post-processing Runtime Post-process: 0.0563905s Solving with MiniSAT 2.2.1 with simplifier 200496 variables, 528343 clausesCBMC Stats: After Upgrade
Runtime Symex: 11.7504s size of program expression: 321064 steps slicing removed 248551 assignments Generated 14256 VCC(s), 5967 remaining after simplification Runtime Postprocess Equation: 0.253811s Passing problem to propositional reduction converting SSA Runtime Convert SSA: 1.44413s Running propositional reduction Post-processing Runtime Post-process: 2.89245s simplifier 1111973 variables, 7270933 clauses SAT checker: instance is SATISFIABLELooking at the generated goto-program for the function
fmt_u64there seems to be two obvious difference:- New division by 0 checks. But in this case the expression is constant, so we could prune them.
- The order of the basic blocks have changed.
New division by 0 checks. But in this case the expression is constant, so we could prune them.
Could we replace the "division-by-zero" checks we currently emit (from CBMC) by these ones? It may be worth considering if the ones coming from Rust include locations (missing from many CBMC checks), but I'm also not sure how they look like.
The order of the basic blocks have changed.
Bad news.
New division by 0 checks. But in this case the expression is constant, so we could prune them.
Could we replace the "division-by-zero" checks we currently emit (from CBMC) by these ones? It may be worth considering if the ones coming from Rust include locations (missing from many CBMC checks), but I'm also not sure how they look like.
Yes. We can definitely get rid of CBMC ones.
The order of the basic blocks have changed.
Bad news.
Yeah, my guess is that this is the root cause of the performance issue. On the bright side, if that is the issue, we can create our own ordering algorithm and make CBMC happy again.
Unfortunately, fixing the basic block order and removing all the new overflow checks were not enough to fix this regression. :(
Reacted by Adrian PalaciosRe-checked with Kani 0.68.0 (CBMC 6.11.0):
kani fmt.rson the harness above now reportsVERIFICATION:- SUCCESSFULwithVerification Time: 9.84s(symex: 54,692 steps, ~225K SAT variables), back in line with the pre-regression numbers rather than ~10 minutes. As a negative control, changing the bound tos.len() <= 3fails as expected. Closing; please reopen if you still see the slowdown.
I tried this code:
using the following command line invocation:
with Kani version: #2551
I expected to see this happen: Verification succeeds in a matter of seconds (before upgrade this harness used to take 6s)
Instead, this happened: Verification took about 10 minutes.