[perf] Optimize FriReducedOpeningChip #1248

nyunyunyunyu · 2025-01-22T15:18:33Z

closes INT-3055

Reduced # of columns of FriReducedOpeningChip from 64 to 26. A side effect is that we use a more efficient way to compute alpha_pow.

Notes for reviewers:

All meaningful changes are in extensions/native/circuit/src/fri/mod.rs. I rewrite everything so diff view is not helpful.
The rows of the new implementation has 3 variants: WorkloadCols/ Instruction1Cols/ Instruction2Cols. I use lots of static assertions in order to guarantee that some columns in these variants are aligned.

WorkloadCols:

1 instruction has length rows of WorkloadCols.
Each row reads a/b from memory and write local.result * local.alpha + b - a into next.result

Instruction1Cols:

1 instruction has only 1 row of Instruction1Cols after all WorkloadCols.
This rows reads a_ptr/b_ptr/length/alpha and write local.result into local.result_ptr.
We constraint a_ptr/b_ptr/alpha in WorkloadCols above are from this row.
This rows also handle the PC interaction of the instruction.

Instruction2Cols:

1 instruction has only 1 row of Instruction2Cols after Instruction1Cols.
This is just for some memory aux columns to reduce the overall width.

fib_e2e before this PR(ref):

Summary	Proof Time (s)	Parallel Proof Time (s)
Total	455.70	316.94
fib_e2e	41.24	6.33
leaf	55.94	8.36
internal.0	56.76	16.20
internal.1	31.91	16.20
internal.2	16.26	16.26
root	39.22	39.22
halo2_outer	168.55	168.55
halo2_wrapper	45.82	45.82

fib_e2e after this PR(ref):

Summary	Proof Time (s)	Parallel Proof Time (s)
Total	433.68	298.21
fib_e2e	40.31	6.25
leaf	54.28	8.29
internal.0	55.42	15.67
internal.1	31.41	15.73
internal.2	15.91	15.91
root	39.70	39.70
halo2_outer	155.79	155.79
halo2_wrapper	40.86	40.86

Proof time of some verifier drops ~30% but others keep same. I'm surprised halo2_outer proof time increases because the number column decreases.

yi-sun · 2025-01-23T02:55:45Z

Can you also update the ISA documentation?

extensions/native/circuit/src/utils.rs

extensions/native/compiler/src/asm/instruction.rs

extensions/native/compiler/tests/fri_ro_eval.rs

docs/specs/ISA.md

github-actions · 2025-01-23T08:41:42Z

group	app.proof_time_ms	app.cycles	app.cells_used	leaf.proof_time_ms	leaf.cycles	leaf.cells_used
verify_fibair	(+113 [+5.0%]) 2,361	513,309	(-28071 [-0.1%]) 19,357,682	-	-	-
fibonacci_program	(+38 [+0.6%]) 6,109	1,500,137	51,487,838	(-94 [-1.2%]) 7,966	(-16002 [-0.9%]) 1,817,697	(-5225391 [-6.9%]) 70,261,826
regex_program	(-703 [-3.8%]) 17,821	4,190,904	165,010,909	(-2029 [-10.6%]) 17,127	(-21360 [-0.7%]) 3,007,473	(-21669219 [-13.3%]) 141,617,522
fib_e2e	40,273	12,000,137	410,699,582	54,976	11,373,351	435,143,222
ecrecover_program	(+5 [+0.2%]) 2,616	285,401	15,075,033	(-1145 [-5.1%]) 21,260	(-28047 [-0.7%]) 4,138,626	(-38606313 [-16.0%]) 202,803,516

Commit: 0ff18c5

Benchmark Workflow

TlatoaniHJ · 2025-01-23T17:33:15Z

extensions/native/circuit/src/fri/mod.rs

+    ) {
+        let local: &Instruction2Cols<AB::Var> = local_slice[..INS_2_WIDTH].borrow();
+        let next: &WorkloadCols<AB::Var> = next_slice[..WL_WIDTH].borrow();
+        {


Not sure why this is necessary, shouldn't it already be covered by lines 302-303?

For example, the last row is a WorkloadCols with idx = -1

Maybe this case will be avoided by other constraints. But this sets an explicit boundary condition just for more safety

I think the case that the last row is a workload row with index -1 is prevented by the fact that an instruction1 row is always followed by an instruction2 row (lines 302-303), an instruction2 row is always followed by a workload row with index 0 (line 403), and a disabled row other than the last is always followed by another disabled row (line 193), so a workload row with index -1 can only be preceded by a workload row with index -2, and so on.

The condition you have here is anyway not preventing workload rows with index -1 that appear in places other than the last row. I guess we can keep it just in case but if you can verify that it's not needed I think it's better to remove to make it less confusing later

TlatoaniHJ · 2025-01-23T17:49:32Z

extensions/native/circuit/src/fri/mod.rs

+            )
+            .eval(builder, multiplicity);
+        {
+            let mut when_transition = builder.when_transition();


Not sure why it's necessary to restrict to when_transition here -- didn't we already restrict that first row has to be the first workload row of a FriReducedOpening, so last row necessary cannot be?

Ah you are right

zlangley · 2025-01-23T17:50:29Z

extensions/native/circuit/src/fri/tests.rs

    a: &[F],
    b: &[[F; EXT_DEG]],
-) -> ([F; EXT_DEG], [F; EXT_DEG]) {
+) -> [F; EXT_DEG] {
+    let mut alpha_pow: [F; EXT_DEG] = elem_to_ext(F::ONE);


nit: Shouldn't we be using EF in general here rather than [F; EXT_DEG]?

What is EF?

Generic for field extension

Hmm how should I change? [F; EXT_DEG] is not a generic

We don't need to change it in this PR. But we would make FriReducedOpeningChip also generic over EF, which would have the trait bound EF: ExtensionField<F>. Anyway, I'm not sure we want to do it; I suppose the AIR stills needs to constrain field extension multiplication for a very specific field extension, not a generic one.

TlatoaniHJ · 2025-01-23T17:59:04Z

I looked through the AIR constraints and it seems to be sound -- I noted two places above where I think there are unnecessary constraints but they are not harmful.

TlatoaniHJ

Will merge, above comments can be addressed in future

nyunyunyunyu added run-benchmark triggers benchmark workflows on the pr run-benchmark-e2e labels Jan 22, 2025

This comment has been minimized.

Sign in to view

nyunyunyunyu force-pushed the feat/opt-fri-reduce branch 2 times, most recently from edc6db6 to 259a25f Compare January 22, 2025 16:24

This comment has been minimized.

Sign in to view

nyunyunyunyu force-pushed the feat/opt-fri-reduce branch 2 times, most recently from 6258838 to ffff287 Compare January 22, 2025 17:13

This comment has been minimized.

Sign in to view

nyunyunyunyu force-pushed the feat/opt-fri-reduce branch from ffff287 to b1e410c Compare January 22, 2025 17:47

nyunyunyunyu marked this pull request as ready for review January 22, 2025 17:48

nyunyunyunyu requested review from TlatoaniHJ and yi-sun January 22, 2025 17:48

nyunyunyunyu force-pushed the feat/opt-fri-reduce branch from b1e410c to 82f0e94 Compare January 22, 2025 17:59

This comment has been minimized.

Sign in to view

yi-sun reviewed Jan 23, 2025

View reviewed changes

Refactor FriReducedOpeningChip

db859fc

jonathanpwang changed the title ~~[chore] Optimize FriReducedOpeningChip~~ [perf] Optimize FriReducedOpeningChip Jan 23, 2025

nyunyunyunyu added 3 commits January 23, 2025 13:08

Remove redundant codes & update ISA docs

51b1611

Reduce degree of FriReducedOpeningChip to 3

4fee261

Revert degree changes

ae71e73

nyunyunyunyu force-pushed the feat/opt-fri-reduce branch from 82f0e94 to ae71e73 Compare January 23, 2025 06:47

Fix lint

d7e4e5e

nyunyunyunyu requested a review from yi-sun January 23, 2025 06:58

This comment has been minimized.

Sign in to view

yi-sun reviewed Jan 23, 2025

View reviewed changes

docs/specs/ISA.md Outdated Show resolved Hide resolved

Update ISA docs

0ff18c5

TlatoaniHJ reviewed Jan 23, 2025

View reviewed changes

zlangley reviewed Jan 23, 2025

View reviewed changes

TlatoaniHJ self-requested a review January 23, 2025 18:29

TlatoaniHJ approved these changes Jan 23, 2025

View reviewed changes

TlatoaniHJ merged commit 1ee9fba into main Jan 23, 2025
23 checks passed

TlatoaniHJ deleted the feat/opt-fri-reduce branch January 23, 2025 18:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[perf] Optimize FriReducedOpeningChip #1248

[perf] Optimize FriReducedOpeningChip #1248

nyunyunyunyu commented Jan 22, 2025 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

yi-sun commented Jan 23, 2025

This comment has been minimized.

github-actions bot commented Jan 23, 2025

TlatoaniHJ Jan 23, 2025

nyunyunyunyu Jan 23, 2025

nyunyunyunyu Jan 23, 2025

TlatoaniHJ Jan 23, 2025

TlatoaniHJ Jan 23, 2025

nyunyunyunyu Jan 23, 2025

zlangley Jan 23, 2025

nyunyunyunyu Jan 23, 2025

yi-sun Jan 23, 2025

nyunyunyunyu Jan 23, 2025

zlangley Jan 23, 2025 •

edited

Loading

TlatoaniHJ commented Jan 23, 2025

TlatoaniHJ left a comment

[perf] Optimize FriReducedOpeningChip #1248

[perf] Optimize FriReducedOpeningChip #1248

Conversation

nyunyunyunyu commented Jan 22, 2025 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

yi-sun commented Jan 23, 2025

This comment has been minimized.

github-actions bot commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zlangley Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

TlatoaniHJ commented Jan 23, 2025

TlatoaniHJ left a comment

Choose a reason for hiding this comment

nyunyunyunyu commented Jan 22, 2025 •

edited

Loading

zlangley Jan 23, 2025 •

edited

Loading