Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add debug.SymbolTable into constraint system for storage efficiency of debug info #421

Merged
merged 6 commits into from
Dec 21, 2022
Merged

feat: add debug.SymbolTable into constraint system for storage efficiency of debug info #421

merged 6 commits into from
Dec 21, 2022

Conversation

gbotrel
Copy link
Collaborator

@gbotrel gbotrel commented Dec 20, 2022 •
edited
Loading

Clearing that from backend; don't store stack trace in string (debug info) in constraint systems, just store ids to a symbol table and format at run time.

Additionally, don't store the stack at all for AssertIsBoolean unless debug build tag is set since we can have a huge amount of these.

Example impact on bn254 pairing:

benchmark                                  old ns/op        new ns/op       delta
BenchmarkFinalExponentiationCompile-10     148012806625     42604127375     -71.22%

benchmark                                  old allocs     new allocs     delta
BenchmarkFinalExponentiationCompile-10     919164251      534771532      -41.82%

benchmark                                  old bytes        new bytes       delta
BenchmarkFinalExponentiationCompile-10     121579914912     78784978536     -35.20%

@gbotrel gbotrel requested a review from ivokub December 20, 2022 20:46
ivokub
ivokub approved these changes Dec 21, 2022
View reviewed changes
Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Only the map for functions in the debug symbol table has filename+methodname as keys, but imo as methodname is fully qualified, then I think can use only methodname. But I think it doesn't make any measurable difference in memory usage, as the debug stacks are way larger anyway.

debug/symbol_table.go
type SymbolTable struct {
Locations []Location
Functions []Function
mFunctions map[string]int `cbor:"-"` // frame.File+frame.Function to id in Functions
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

frame.Function is fully qualified name of the function. Do we need to store also the file name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is derived from the profile/ package which is itself derived from go std, don't think it matters too much indeed :)

@gbotrel gbotrel merged commit 24fe00b into develop Dec 21, 2022
@gbotrel gbotrel deleted the perf/debuginfo branch December 21, 2022 15:09
ivokub added a commit that referenced this pull request Feb 17, 2023
@ivokub
Squashed commit of the following:
e57b0a6 
commit 82eeb20
Author: Gautam Botrel <gautam.botrel@gmail.com>
Date:   Thu Dec 22 13:13:01 2022 -0600

    fix: don't overwrite buffer when compressing le

commit 6fc0594
Author: Gautam Botrel <gautam.botrel@gmail.com>
Date:   Thu Dec 22 13:10:07 2022 -0600

    revert: revert builder.linearExpression method

commit e7711d3
Author: Gautam Botrel <gautam.botrel@gmail.com>
Date:   Thu Dec 22 13:03:03 2022 -0600

    style: clean PR from NewBuffer

commit 00361d8
Merge: af31f2e 66e4fab
Author: Gautam Botrel <gautam.botrel@gmail.com>
Date:   Thu Dec 22 12:51:29 2022 -0600

    Merge remote-tracking branch 'origin/develop' into feat/apiMAC

commit af31f2e
Author: Gautam Botrel <gautam.botrel@gmail.com>
Date:   Thu Dec 22 12:51:13 2022 -0600

    feat: add api.MAC and api.Compiler().NewBuffer(capacity)

commit 66e4fab
Author: Gautam Botrel <gautam.botrel@gmail.com>
Date:   Thu Dec 22 12:40:02 2022 -0600

    Restore frontend.WithCapacity (to merge after perf/debugInfo) (#423)

    * feat: add debug.SymbolTable into constraint system for storage efficiency of debug info

    * feat: solution takes symbol table only, limit stack size to 2 without debug

    * perf: don't store stack for assert boolean if no debug flag

    * perf: restore frontend.WithCapacity usage

    * fix: fix line number in Example output

    * fix: remove boolean trace test

commit 4718886
Merge: 2a0ef4c 24fe00b
Author: Gautam Botrel <gautam.botrel@gmail.com>
Date:   Wed Dec 21 09:15:45 2022 -0600

    Merge branch 'develop' into feat/apiMAC

commit 24fe00b
Author: Gautam Botrel <gautam.botrel@gmail.com>
Date:   Wed Dec 21 09:09:41 2022 -0600

    feat: add debug.SymbolTable into constraint system for storage efficiency of debug info (#421)

    * feat: add debug.SymbolTable into constraint system for storage efficiency of debug info

    * feat: solution takes symbol table only, limit stack size to 2 without debug

    * perf: don't store stack for assert boolean if no debug flag

    * fix: fix line number in Example output

    * fix: remove boolean trace test

commit 2a0ef4c
Author: Gautam Botrel <gautam.botrel@gmail.com>
Date:   Tue Dec 20 17:58:06 2022 -0600

    experiment checkpoint

commit b3778b3
Author: Gautam Botrel <gautam.botrel@gmail.com>
Date:   Tue Dec 20 15:59:10 2022 -0600

    autogenerate tinyfield arithmetic (#424)

    * feat: auto generate tiny field

    * fix: update toBigInt interface for conversion element - big int

    * fix: revert previous commit

commit 739a2d9
Merge: 35dccd5 2128e51
Author: Gautam Botrel <gautam.botrel@gmail.com>
Date:   Tue Dec 20 15:17:04 2022 -0600

    Merge branch 'perf/debuginfo' into perf/capacity

commit 2128e51
Author: Gautam Botrel <gautam.botrel@gmail.com>
Date:   Tue Dec 20 15:16:53 2022 -0600

    fix: remove boolean trace test

commit 19312ea
Merge: 1ab5fff 908eb80
Author: Gautam Botrel <gautam.botrel@gmail.com>
Date:   Tue Dec 20 15:15:04 2022 -0600

    Merge branch 'develop' into perf/debuginfo

commit 908eb80
Author: Arya Tabaie <arya.pourtabatabaie@gmail.com>
Date:   Tue Dec 20 15:14:35 2022 -0600

    Reflect PRs in gnark-crypto: A field element is always in Montgomery form and big.Ints are always non-Mont (#422)

    * refactor: no non-mont on bls12-377

    * refactor: groth16 backend tests pass

    * refactor: MSM takes Montgomery only - Plonk

    * fix:`ToBigIntRegular` => `BigInt`

    * fix: more `ToBigIntRegular` => `BigInt`

commit 35dccd5
Merge: 480df50 1ab5fff
Author: Gautam Botrel <gautam.botrel@gmail.com>
Date:   Tue Dec 20 14:59:04 2022 -0600

    Merge branch 'perf/debuginfo' into perf/capacity

commit 1ab5fff
Author: Gautam Botrel <gautam.botrel@gmail.com>
Date:   Tue Dec 20 14:58:53 2022 -0600

    fix: fix line number in Example output

commit 480df50
Author: Gautam Botrel <gautam.botrel@gmail.com>
Date:   Tue Dec 20 14:58:21 2022 -0600

    perf: restore frontend.WithCapacity usage

commit f918071
Author: Gautam Botrel <gautam.botrel@gmail.com>
Date:   Tue Dec 20 14:44:20 2022 -0600

    perf: don't store stack for assert boolean if no debug flag

commit 1204a89
Author: Gautam Botrel <gautam.botrel@gmail.com>
Date:   Tue Dec 20 14:40:40 2022 -0600

    feat: solution takes symbol table only, limit stack size to 2 without debug

commit 2c0390a
Author: Gautam Botrel <gautam.botrel@gmail.com>
Date:   Tue Dec 20 14:10:58 2022 -0600

    feat: add debug.SymbolTable into constraint system for storage efficiency of debug info
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivokub ivokub ivokub approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gbotrel @ivokub