perf(keccak-circuits): preallocate and bulk realloc memory where possible

[bishopcheckmate]

Description

This change optimizes memory allocations in keccak_packed_multi. Where possible, pre-allocates needed memory so later when .push()s happen, vectors don't need to waste time on re-allocating and moving contents. This should also optimize the memory footprint slightly as the vec's growth strategy could over-allocate at some point.

Type of change

• Perf optimization, refactor (non-breaking change which optimizes code's performance / memory footprint)
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Contents

• applied Vec::with_capacity where the capacity is known a priori
• used Vec::resize instead of repeating .push()es where possible

Rationale

Reserving space in advance is more performant.

How Has This Been Tested?

cargo test -p zkevm-circuits -- keccak

[ChihChengLiang]

Hey @bishopcheckmate,

First off, thanks for your contribution! I appreciate your effort. However, I have some concerns about the changes you've proposed:

• Unclear Benefits: It would be great if we could run some benchmarks to understand the performance improvements better before diving into these optimizations.
• Clarity Priority: Our codebase is already quite complex, and at this point, I think prioritizing clarity is crucial. Managing vector resizing and capacity might add more confusion for developers trying to grasp the core logic.

My suggestion would be to explore a functional refactor, i.e. using iterators to avoid mutating vectors, that can give us both clarity and performance gains.

[bishopcheckmate]

Hey, thanks for the reply.

Unclear Benefits: It would be great if we could run some benchmarks to understand the performance improvements better before diving into these optimizations.

I can explore the benches you have there more, however I was already running some and experienced that there are some running even more than an hour. My machine is not really a good benchmark suite for zk stuff as it tends to be too heavy to avoid the regular noise to make results not helpful at all. Eg. it's not uncommon for me to run such long benchmark twice and observe +-15% difference on unchanged code. If I'd like to see a gains of the changes like those, I think it'd be worth to explore some micro-benchmarking instead.

My reasoning there was rather like pre-allocs and resizes are definitely more performant, and if we would apply them throughout the whole codebase, then they can add-up to give nice gains as a whole, and this change was only a first batch of those.

Clarity Priority: Our codebase is already quite complex, and at this point, I think prioritizing clarity is crucial. Managing vector resizing and capacity might add more confusion for developers trying to grasp the core logic.

I agree the clarity should always be priority and I don't think you can avoid complexity so definitely things shouldn't be made more complex without purpose. Where I disagree tho is that for me the resize for a known size instead of looping on some condition makes it easier to reason, like 'now we insert columns+1 zero fields' instead of 'now we add zero fields until the length is higher than columns'. Also Vec::with_capacity in my opinion makes it easier to reason about how this vector is intended to be used or for what, and how will it look after the function.
Tho I agree this resize is pretty cryptic 😅

[ed255]

Based on the comments from @ChihChengLiang in #1732 (review) we decided to close this PR for now; but we're open to receiving new PRs on this topic if:

• Benchmarks show an improvement
• The code is clear