feat: location tree for debug_info #7034

guipublic · 2025-01-13T11:30:09Z

Description

Problem*

Resolves #6946

Summary*

Adds the location tree to debug info and reference call-stacks through their 'index' in the tree (i.e a node of the tree).

Additional Context

The location tree is merged from ACIR location tree and brillig per function location trees.
The PR is a draft because it does not merge the trees in case of several ACIR functions, and also one flame graph test is failing in Noir debugger.

Documentation*

Check one:

No documentation needed.
Documentation included in this PR.
[For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

I have tested the changes locally.
I have formatted the changes with Prettier and/or cargo fmt on default settings.

github-actions · 2025-01-13T12:04:04Z

Compilation Memory Report

Program	Peak Memory	%
keccak256	77.63M	0%
workspace	123.94M	0%
regression_4709	424.14M	0%
ram_blowup_regression	1.46G	0%
rollup-root	597.10M	77%
rollup-merge	493.80M	115%
rollup-block-root-single-tx	16.05G	6%
rollup-block-root-empty	487.97M	118%
rollup-block-root	16.06G	6%
rollup-block-merge	597.09M	77%
rollup-base-public	2.33G	0%
rollup-base-private	1.11G	100%
private-kernel-tail	206.15M	-1%
private-kernel-reset	567.56M	-3%
private-kernel-inner	293.62M	-1%

github-actions · 2025-01-13T12:04:04Z

Execution Memory Report

Program	Peak Memory	%
keccak256	74.68M	0%
workspace	123.97M	0%
regression_4709	316.00M	0%
ram_blowup_regression	512.46M	0%
rollup-root	494.81M	114%
rollup-merge	472.73M	130%
rollup-block-root	1.08G	0%
rollup-block-merge	494.82M	114%
rollup-base-public	667.94M	39%
rollup-base-private	562.61M	73%
private-kernel-tail	179.42M	-1%
private-kernel-reset	228.64M	-7%
private-kernel-inner	203.62M	-3%

github-actions · 2025-01-14T09:22:14Z

Execution Report

Program	Execution Time	%
sha256_regression	0.051s	0%
regression_4709	0.001s	0%
ram_blowup_regression	0.620s	3%
rollup-root	0.104s	0%
rollup-merge	0.007s	16%
rollup-block-root	37.300s	1%
rollup-block-merge	0.104s	-3%
rollup-base-public	1.220s	-1%
rollup-base-private	0.453s	0%
private-kernel-tail	0.019s	0%
private-kernel-reset	0.314s	0%
private-kernel-inner	0.070s	4%

github-actions · 2025-01-14T09:22:14Z

Compilation Report

Program	Compilation Time	%
sha256_regression	1.020s	-5%
regression_4709	0.809s	-1%
ram_blowup_regression	16.000s	-2%
rollup-root	3.534s	5%
rollup-merge	2.060s	6%
rollup-block-root-single-tx	140.000s	0%
rollup-block-root-empty	2.028s	3%
rollup-block-root	138.000s	-7%
rollup-block-merge	3.644s	8%
rollup-base-public	26.520s	-8%
rollup-base-private	9.696s	-3%
private-kernel-tail	0.956s	2%
private-kernel-reset	5.890s	-4%
private-kernel-inner	2.228s	11%

TomAFrench · 2025-01-14T17:41:25Z

Merging in master to compare artifact sizes in aztec-packages.

TomAFrench · 2025-01-14T17:56:15Z

Nice, this drops the artifact size of rollup-block-root-single-tx by ~1MB from 20 to 19. Compilation time drops by 50s

compiler/wasm/src/types/noir_artifact.ts

compiler/noirc_evaluator/src/acir/acir_variable.rs

TomAFrench

Forgot to hit send on these comments.

Also aztec-packages has their own homebrew method of dealing with debug info in their simulator which is this is definitely going to break. Can you take a look at what this will take to update?

compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs

compiler/noirc_evaluator/src/ssa/opt/unrolling.rs

compiler/noirc_errors/src/call_stack.rs

tooling/debugger/src/context.rs

compiler/noirc_evaluator/src/brillig/mod.rs

TomAFrench · 2025-01-15T15:03:45Z

compiler/noirc_errors/src/debug_info.rs

+    pub brillig_locations:
+        BTreeMap<BrilligFunctionId, BTreeMap<BrilligOpcodeLocation, CallStackId>>,
+    pub location_tree: LocationTree,
    /// Map opcode index of an ACIR circuit into the source code location
    /// Serde does not support mapping keys being enums for json, so we indicate
    /// that they should be serialized to/from strings.
    #[serde_as(as = "BTreeMap<DisplayFromStr, _>")]
-    pub locations: BTreeMap<OpcodeLocation, Vec<Location>>,
-    pub brillig_locations:
-        BTreeMap<BrilligFunctionId, BTreeMap<BrilligOpcodeLocation, Vec<Location>>>,
+    pub location_map: BTreeMap<OpcodeLocation, CallStackId>,


Can you give an explainer on how exactly these fit together? I'm thinking that we're storing unnecessary data in location_map now.

My understanding is that:

brillig_locations stores for each brillig function a mapping from each opcode index to a callstack id

We can then use this and then look up the callstack id in location_tree to get the associate callstack for any brillig opcode.

location_map stores a mapping from (acir_opcode_index, brillig_index) to a callstack id which we can use with location_tree.

Why do we need to track (acir_opcode_index, brillig_index) in location_map now? If we fail inside of a brillig call we can tell which brillig function we're executing from the ACIR opcode we've halted on, we then

Get the callstack for the ACIR opcode in which we make the call

Check the brillig opcode within that call we halted on and get the relevant callstack for the unconstrained function.

Smoosh these two together to generate the final callstack.

In theory this makes sense, in practice I don't see where those brillig locations are inserted in the location_map.

Hmm, true we seem to have removed the insertion of any OpcodeLocation::Brillig a while back.

It still stands that we don't need to keep this enum in the build artifact anymore though as it's currently tech debt. We've kept this enum around just to keep the program serialization format the same but as we're changing it now anyway, now's a good time to clear up this tech debt.

noir/acvm-repo/acir/src/circuit/mod.rs

Lines 139 to 147 in 0512ee6

#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)]

/// Opcodes are locatable so that callers can

/// map opcodes to debug information related to their context.

pub enum OpcodeLocation {

Acir(usize),

// TODO(https://github.com/noir-lang/noir/issues/5792): We can not get rid of this enum field entirely just yet as this format is still

// used for resolving assert messages which is a breaking serialization change.

Brillig { acir_index: usize, brillig_index: usize },

}

#5792

guipublic added 4 commits January 13, 2025 11:22

location tree for debug_info

cf0eeb9

forgot to commit files from tooling

17ff57d

cargo.lock

eeb1e9f

Merge branch 'master' into gd/issue_6946

aa74a7b

guipublic and others added 3 commits January 13, 2025 12:05

fix test case

0984da7

set call_stack_id during acir-gen

a3eb219

Merge branch 'master' into gd/issue_6946

fb962e0

guipublic and others added 5 commits January 14, 2025 15:53

update ts DebugInfo

3548030

update comment

70f5563

Merge branch 'master' into gd/issue_6946

9c06c93

format

ca52a8e

Merge branch 'master' into gd/issue_6946

4283acf

put call stack in correct order

8683746

guipublic marked this pull request as ready for review January 14, 2025 18:04

guipublic requested a review from a team January 14, 2025 18:11

Merge branch 'master' into gd/issue_6946

354b075

TomAFrench reviewed Jan 15, 2025

View reviewed changes

compiler/wasm/src/types/noir_artifact.ts Show resolved Hide resolved

TomAFrench reviewed Jan 15, 2025

View reviewed changes

compiler/noirc_evaluator/src/acir/acir_variable.rs Outdated Show resolved Hide resolved

code review

b3332d5

TomAFrench reviewed Jan 15, 2025

View reviewed changes

TomAFrench and others added 4 commits January 15, 2025 22:43

Merge branch 'master' into gd/issue_6946

22357be

code review

41a9c09

code review

60a4fdc

Merge branch 'master' into gd/issue_6946

6b38614

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: location tree for debug_info #7034

feat: location tree for debug_info #7034

guipublic commented Jan 13, 2025

github-actions bot commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 14, 2025 •

edited

Loading

github-actions bot commented Jan 14, 2025 •

edited

Loading

TomAFrench commented Jan 14, 2025

TomAFrench commented Jan 14, 2025

TomAFrench left a comment

TomAFrench Jan 15, 2025

guipublic Jan 16, 2025

TomAFrench Jan 16, 2025

	#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)]
	/// Opcodes are locatable so that callers can
	/// map opcodes to debug information related to their context.
	pub enum OpcodeLocation {
	Acir(usize),
	// TODO(https://github.com/noir-lang/noir/issues/5792): We can not get rid of this enum field entirely just yet as this format is still
	// used for resolving assert messages which is a breaking serialization change.
	Brillig { acir_index: usize, brillig_index: usize },
	}

feat: location tree for debug_info #7034

Are you sure you want to change the base?

feat: location tree for debug_info #7034

Conversation

guipublic commented Jan 13, 2025

Description

Problem*

Summary*

Additional Context

Documentation*

PR Checklist*

github-actions bot commented Jan 13, 2025 • edited Loading

Compilation Memory Report

github-actions bot commented Jan 13, 2025 • edited Loading

Execution Memory Report

github-actions bot commented Jan 14, 2025 • edited Loading

Execution Report

github-actions bot commented Jan 14, 2025 • edited Loading

Compilation Report

TomAFrench commented Jan 14, 2025

TomAFrench commented Jan 14, 2025

TomAFrench left a comment

Choose a reason for hiding this comment

TomAFrench Jan 15, 2025

Choose a reason for hiding this comment

guipublic Jan 16, 2025

Choose a reason for hiding this comment

TomAFrench Jan 16, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 14, 2025 •

edited

Loading

github-actions bot commented Jan 14, 2025 •

edited

Loading