Use abstract rings for Zip and linear code - #2
Conversation
|
Coverage after merging feature/zip-rings into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Coverage after merging feature/zip-rings into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Coverage after merging feature/zip-rings into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
frozenspider
changed the title
|
Coverage after merging feature/zip-rings into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
cupicmarko
left a comment
cupicmarko
left a comment
There was a problem hiding this comment.
I didn't finish the review yet, but I'll mark the PR approved once I finish, I don't see any changes to request or any questions. If performance are the same or better, this is a nicer, cleaner code.
I would reconsider #[inline(always)] flags, because they can increase the resulting code size, which is usually fine, but it can affect performance on CPUs with small instruction cache (Cortex-M can have 8-16KB for example), so I would suggest using #[inline] and letting the compiler decide on inlining. I don't know how real this scenario is, just mentioning it.
| } | ||
|
|
||
| #[allow(clippy::arithmetic_side_effects)] | ||
| fn accumulate_unchecked<I>(input: &mut [I]) |
There was a problem hiding this comment.
Based on the name, I would suggest marking the function unsafe and removing unsafe block inside.
There was a problem hiding this comment.
This function is not unsafe in the usual Rust sense (no undefined behaviour). I agree that it makes sense to mark function unsafe for internal reasons too, but in this case this intention is hard to propagate. Making it unsafe will just make encode_inner to use unsafe call wrapper twice - however, both it and accumulate_unchecked are internal functions not visible to the outside world, so it doesn't feel like we're accomplishing much.
On the other hand, unsafe block inside this function is unsafe in memory manipulation sense, so it makes sense to keep unsafe {} block wrapper there.
Overall, I think in this case I'd prefer to keep it the way it is now.
There was a problem hiding this comment.
I see your point, can we add debug_asserts that check for the UB?
There was a problem hiding this comment.
Asked a follow-up question on Slack, let's do it in the next PR
|
Coverage after merging feature/zip-rings into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Thanks @cupicmarko! |
maksimryndin
left a comment
maksimryndin
left a comment
There was a problem hiding this comment.
Awesome work, @frozenspider ! 👍
Probably, in future optimizations it is worth also to try an alternative to crypto-bigint
https://github.com/recmo/uint by Remco et al
That seems good, although, I would assume that the compiler is already doing it on its own. It's not something that is needed to be changed, just NIT. |
cupicmarko
left a comment
cupicmarko
left a comment
There was a problem hiding this comment.
Looks good! Just some NIT comments, nothing else
…aryPoly) Optimization #2 (eq-weighted higher-degree path): - New evaluate_combined_polynomials_at_point: shift-spec support, last-row Lagrange correction, replaces evaluate_combined_polynomials_linear - New compute_combined_polynomial_evals_with_eq: eq-weight accumulation for higher-degree constraints, avoids O(K×D×N) intermediate MLE storage - prove_at_point dispatches: degree≤1 → at_point, degree>1 → eq-weighted - scale_dynamic_poly_f helper, backward-compat linear alias Optimization #3 (fused BinaryPoly → constraint evaluation): - New prove_from_binary_poly / prove_from_binary_poly_at_point entry points operating directly on BinaryPoly<D> traces for any constraint degree - New compute_combined_evals_from_binary_poly_with_eq: fuses projection + constraint evaluation + eq-weighted accumulation into a single pass, eliminating the full projected trace allocation (num_cols × N × D)
Combines optimizations #2 and #3 from the perf analysis. Replaces the hashmap-based multiplicity index AND drops the intermediate `chunks_psi: Vec<Vec<Vec<F>>>` materialization in `prove_group`. Before: Step 1: chunks_psi[ell][k][i] = ψ_a-projection of K·W bit-polys, each evaluated as `Σ_p bit_p · a^p` — L·K·W·cw field adds. Step 2: build_table_index → HashMap<Vec<u8>, usize>. Step 3: per (ell, chunk) call compute_multiplicities_with_index → W hash lookups per chunk × L·K calls = L·K·W hashmap lookups (~2M at L=8, K=4, W=2^16). Step 5: leaf_q[…] = β − chunks_psi[ell][k][i]; reads through the L·K·W F intermediates a second time. After: Step 1: chunks_idx[ell][k][i]: u32 — extract cw bits from the parent BitPoly via OR (no field arithmetic). Step 2: subtable only; no hashmap. Step 3: agg_mults[ell][n] = histogram of chunks_idx[ell] flattened into Vec<u64>, then converted to F. Pure integer work. Step 5: precompute β_minus_subtable: Vec<F> of length 2^cw, then leaf_q[…] = β_minus_subtable[chunks_idx[ell][k][i]] — one F clone per leaf, no per-leaf field op. Net effect: - L·K·W hashmap lookups → L·K·W array indexes (~50ns → ~1ns each) - L·K·W field adds (chunks_psi build) → L·K·W·cw bit ORs - L·K·W F clones for chunks_psi → none (chunks_psi gone) - β_minus_subtable amortizes the β subtraction over 2^cw entries instead of L·K·W leaves Pure-internal optimization: no FS transcript or proof byte changes. 17 protocol + 60 piop tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 participants
Instead of
N, L, K, Musize constant bounds forInts, both Zip and linear code now operates on 4 abstract rings:N => EvaluationRingL(previously: element width of the encoding matrix of a linear code) is no longer a generic, instead it's made into a property of linear code.PrimeFieldelements must be obtainable from it, but otherwise code is free to place any constraints on the rings that it needs to make it work.K => CodewordRingChallengeRing, used to generate challenges for random linear combination of codewords. Previously it was the same as evaluation ring.M => LinearCombinationRingThese ring aliases are implemented by
Int<_>rings, so not much changes from a user perspective, except you have to specifyChallengeRingseparately.Other major changes
MulByScalarto represent an ability to multiply a given ring element byChallengeRing(and field elements with each other, as they are being combined too)LinearCodeno longer has genericencode_wide, instead 3 encode methods are now defined as follows:encode: EvaluationRing => CodewordRingencode_wide: LinearCombinationRing => LinearCombinationRingencode_f: PrimeField => PrimeFieldRaaCodeto have a configurablecheck_for_overflows. This is needed to keep encoding times on par with original Zinc encoding times - usingchecked_addadds ~20% overhead.Transcribable. It can use whatever byte order as long as it allows serialization and deserialization cross-platform. (Actually uses little-endian order as being more efficient and more natural when applied to underlyingcrypto-biginttypes)AsWordstrait unnecessary, removed itAdditional changes
ConstRingis now auto-implementeddensemultilinear polynomial module moved frompolytopoly::mleto free up module namenum_proximity_testingno longer depends onlog2_q. It was unused previously anyway, and if we need it in future, we'd need to determine whatlog2_qis for an arbitrary ring.Performance
(single-threaded)