fix(ssa_interpreter): push_back and pop_back to slices with padding#9320
fix(ssa_interpreter): push_back and pop_back to slices with padding#9320
push_back and pop_back to slices with padding#9320Conversation
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 2302d21 | Previous: 42429ab | Ratio |
|---|---|---|---|
sha512-100-bytes |
1.969 s |
1.592 s |
1.24 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 2302d21 | Previous: 42429ab | Ratio |
|---|---|---|---|
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib |
2 s |
1 s |
2 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib |
4 s |
2 s |
2 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types |
2 s |
1 s |
2 |
test_report_zkpassport_noir-ecdsa_ |
2 s |
1 s |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
push_back and pop_back to slices with paddingpush_back and pop_back to slices with padding
I wonder if this could be related to any of the performance issues with slices. Maybe this could happen if e.g. you have a |
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE fix: forbid self-referencing type aliases (noir-lang/noir#9103) chore: add a mem2reg test for when all references need to be invalidated (noir-lang/noir#9377) fix(ssa): Do not check ArrayGet/Set as unreachable for Brillig (noir-lang/noir#9376) chore: use SSA parser in all mem2reg tests (noir-lang/noir#9372) fix: trait where clause check fixes (noir-lang/noir#9369) fix: Correct doc comments for SSA passes (noir-lang/noir#9371) fix: prevent `SignedField::from(i128::MIN)` from crashing (noir-lang/noir#9366) fix: allow constants in the type-system to be negative (noir-lang/noir#9360) feat: show circuit output as a value of the program's return type (noir-lang/noir#9364) feat: add `FunctionDefinition::visibility` (noir-lang/noir#9363) chore(docs): Add example for `$crate` in docs (noir-lang/noir#9361) fix: Prevent accidental tuple sharing in comptime code (noir-lang/noir#9313) fix: perserve purities after SSA normalization (noir-lang/noir#9355) fix: modulo overflow in comptime (noir-lang/noir#9348) fix: handle short-syntax for trait constraints on trait generics (noir-lang/noir#9167) chore: enhance trait constraint comment (noir-lang/noir#9358) fix: replace implicitly added named generics with fresh type vars in check_trait_impl_where_clause_matches_trait_where_clause (noir-lang/noir#9352) fix: push definition trait constraints after trait item constraint (noir-lang/noir#9354) chore(ci): Update status of noir_json_parser (noir-lang/noir#9351) fix(ssa): Keep reference count increments for array set values (noir-lang/noir#9344) chore: remove unused `compile_workspace` (noir-lang/noir#9353) chore: try printing byte arrays as strings in the SSA interpreter (noir-lang/noir#9346) feat(lsp): allow opening noir stdlib files (noir-lang/noir#9339) fix: do u128 operations with u128, not i128 (noir-lang/noir#9345) chore(acir): ACIR parser error handling for blackbox inputs/outputs (noir-lang/noir#9342) fix: prevent invalid types in test/fuzz functions (noir-lang/noir#9343) chore(lsp): avoid redundant type checking (noir-lang/noir#9337) feat(acir): Parse ACIR memory and call opcodes (noir-lang/noir#9331) fix(ssa_gen): Add constraint on slice length before popping (noir-lang/noir#9323) chore: impl for u16 conversions (noir-lang/noir#9314) fix: substitute bindings in type before canonicalization (noir-lang/noir#9328) fix(ssa_interpreter): `push_back` and `pop_back` to slices with padding (noir-lang/noir#9320) fix: wildcard type should be allowed in lambda parameter types (noir-lang/noir#9325) chore: graceful handling of SIGPIPE (noir-lang/noir#9075) feat: return unsolvable opcode from `CircuitSimulator` (noir-lang/noir#8943) fix: allow nested fmtstr (noir-lang/noir#9309) feat: Initial ACIR parser (arithmetic exprs and black box functions) (noir-lang/noir#9316) fix(mem2reg): Register aliases when the `IfElse` result in a reference (noir-lang/noir#9305) fix: Make Ssa-gen use existing reference when compiling `&mut foo.bar.baz` (noir-lang/noir#9307) fix: top-level item in dependency isn't always visible (noir-lang/noir#9295) fix(ssa-interpreter): Return error if slice length is 0 during popping (noir-lang/noir#9308) chore: Release Noir(1.0.0-beta.9) (noir-lang/noir#9184) chore(LSP): simplify code lens request handling (noir-lang/noir#9279) chore: add regression tests for #6383 (noir-lang/noir#9302) fix: disallow `_` in signatures and struct members (noir-lang/noir#9301) fix: check associated types after validating where clause when looking up trait impls, plus some unification fixes (noir-lang/noir#9265) chore: Add fmtstr to coercions list (noir-lang/noir#9300) chore: Add a helper function `fmtstr::as_quoted_str` (noir-lang/noir#9293) chore(docs): Copy Type Coercions docs into v1.0.0-beta.8 versioned docs (noir-lang/noir#9298) feat: only inject "out of bounds" checks in brillig (noir-lang/noir#9200) END_COMMIT_OVERRIDE --------- Co-authored-by: AztecBot <tech@aztecprotocol.com> Co-authored-by: Maxim Vezenov <mvezenov@gmail.com>
Description
Problem*
Resolves #9312
Summary*
Change
slice_push_backandslice_push_frontto take into account any potential extra items in the slice which are beyond itslength. Push and pop must operate before this region. This is achieved by removing any potential extras, since they are never accessed by the interpreter.Additional Context
This situation arises when we have something like this code:
The SSA pass that removes the
IfElseexpressions creates a slice like this:If
pis true,cends up looking like[1,2,5,6], and after pushing we want it to be(3, [1,2,7,5,6]), or if we pop then it should return(1, [1,5,6], 2)(length, data, elem).Initially I appended the extra items back onto the slice, but recognised some alternatives:
push_back, rather than grow the slice, to reclaim space; not like the interpreter is necessarily built to be efficient, just to maybe match what ACIR or Brillig might do.I checked what ACIR passes to the print oracle after a push/pop, but it doesn't looks like something the interpreter aims to match:
The values passed to the foreign function look like this:
So popping didn't make the array shorter, just decreased the length, and pushing both filled the extra element and added a new one.
For this reason I concluded that the extra elements shouldn't be relied on to have any particular content, and it's best if we truncate the slices in the interpreter during pushing and popping, rather than go the extra mile to keep the extra elements. I didn't update other slice operations though.
Documentation*
Check one:
PR Checklist*
cargo fmton default settings.