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

LEM circuit refactor #1184

Merged
merged 6 commits into from
Mar 2, 2024
Merged

LEM circuit refactor #1184

merged 6 commits into from
Mar 2, 2024

Conversation

gabriel-barrett
Copy link
Member

This PR solves the first point in #1051. Instead of always pre-allocating output pointers we choose to do this based on whether there's a branch or not. This makes the use of auxiliary funcs cheaper. Not very important for the eval step right now, but it can be a significant improvement for small funcs, specially in the context of coroutines.

@gabriel-barrett gabriel-barrett requested review from a team as code owners February 29, 2024 00:37
arthurpaulino
arthurpaulino reviewed Feb 29, 2024
View reviewed changes
src/lem/circuit.rs Outdated Show resolved Hide resolved
src/lem/circuit.rs Outdated Show resolved Hide resolved
src/lem/circuit.rs Outdated Show resolved Hide resolved
src/lem/circuit.rs Outdated Show resolved Hide resolved
src/lem/circuit.rs Outdated Show resolved Hide resolved
src/lem/circuit.rs Show resolved Hide resolved
src/lem/circuit.rs Outdated Show resolved Hide resolved
arthurpaulino
arthurpaulino reviewed Feb 29, 2024
View reviewed changes
src/lem/circuit.rs Outdated Show resolved Hide resolved
@gabriel-barrett
Copy link
Member Author

!benchmark --features cuda

@github-actions GitHub Actions
Copy link
Contributor

Benchmark for 313180a

Click to view benchmark
Test Base PR %
LEM Fibonacci Prove - rc = 100/fib/num-100 1458.2±11.71ms 1490.1±8.02ms +2.19%
LEM Fibonacci Prove - rc = 100/fib/num-200 2.8±0.02s 2.9±0.01s +3.57%
LEM Fibonacci Prove - rc = 600/fib/num-100 1848.2±11.46ms 1889.3±16.30ms +2.22%
LEM Fibonacci Prove - rc = 600/fib/num-200 3.1±0.02s 3.1±0.02s 0.00%

@github-actions GitHub Actions
Copy link
Contributor

!gpu-benchmark action succeeded! 🚀

https://github.com/lurk-lab/lurk-rs/actions/runs/8098910446

@arthurpaulino
Copy link
Member

!benchmark --features cuda

@github-actions GitHub Actions
Copy link
Contributor

Benchmark for 313180a

Click to view benchmark
Test Base PR %
LEM Fibonacci Prove - rc = 100/fib/num-100 1464.6±14.65ms 1497.5±13.27ms +2.25%
LEM Fibonacci Prove - rc = 100/fib/num-200 2.8±0.02s 2.9±0.01s +3.57%
LEM Fibonacci Prove - rc = 600/fib/num-100 1861.5±12.73ms 1873.5±12.72ms +0.64%
LEM Fibonacci Prove - rc = 600/fib/num-200 3.0±0.01s 3.1±0.02s +3.33%

@github-actions GitHub Actions
Copy link
Contributor

!gpu-benchmark action succeeded! 🚀

https://github.com/lurk-lab/lurk-rs/actions/runs/8099193893

arthurpaulino
arthurpaulino requested changes Feb 29, 2024
View reviewed changes
Copy link
Member

@arthurpaulino arthurpaulino left a comment

Choose a reason for hiding this comment

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

Unfortunately the slowdown seems to be consistent so I'm blocking the merge.
Meanwhile, there are other minor changes on this PR that could be factored out to a smaller PR

@gabriel-barrett
Copy link
Member Author

!benchmark --features cuda

@github-actions GitHub Actions
Copy link
Contributor

Benchmark for 867f0c2

Click to view benchmark
Test Base PR %
LEM Fibonacci Prove - rc = 100/fib/num-100 1462.0±8.51ms 1484.8±5.06ms +1.56%
LEM Fibonacci Prove - rc = 100/fib/num-200 2.8±0.01s 2.8±0.01s 0.00%
LEM Fibonacci Prove - rc = 600/fib/num-100 1844.5±4.83ms 1898.5±14.62ms +2.93%
LEM Fibonacci Prove - rc = 600/fib/num-200 3.0±0.01s 3.1±0.02s +3.33%

@github-actions GitHub Actions
Copy link
Contributor

!gpu-benchmark action succeeded! 🚀

https://github.com/lurk-lab/lurk-rs/actions/runs/8101723638

@gabriel-barrett
Copy link
Member Author

!benchmark --features cuda

@github-actions GitHub Actions
Copy link
Contributor

Benchmark for 867f0c2

Click to view benchmark
Test Base PR %
LEM Fibonacci Prove - rc = 100/fib/num-100 1453.8±7.84ms 1454.7±8.57ms +0.06%
LEM Fibonacci Prove - rc = 100/fib/num-200 2.8±0.01s 2.8±0.02s 0.00%
LEM Fibonacci Prove - rc = 600/fib/num-100 1855.2±13.20ms 1829.5±12.49ms -1.39%
LEM Fibonacci Prove - rc = 600/fib/num-200 3.0±0.01s 3.0±0.02s 0.00%

@github-actions GitHub Actions
Copy link
Contributor

!gpu-benchmark action succeeded! 🚀

https://github.com/lurk-lab/lurk-rs/actions/runs/8103161782

porcuquine
porcuquine approved these changes Feb 29, 2024
View reviewed changes
Copy link
Collaborator

@porcuquine porcuquine left a comment

Choose a reason for hiding this comment

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

Approved contingent on @arthurpaulino's sign-off.

arthurpaulino
arthurpaulino approved these changes Mar 1, 2024
View reviewed changes
src/lem/circuit.rs

/// The collection of all return values and `not_dummy`s of all
/// branches in a block.
struct BranchOutputs<F: LurkField>(Vec<(Boolean, Vec<AllocatedPtr<F>>)>);
Copy link
Member

@arthurpaulino arthurpaulino Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Nits:

  • I think a better name for this is either BlockOutputs or BranchesOutputs (I prefer the former).
  • Also, why wrap it with a struct? A type alias would do

@gabriel-barrett gabriel-barrett added this pull request to the merge queue Mar 1, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Mar 1, 2024
@gabriel-barrett gabriel-barrett added this pull request to the merge queue Mar 2, 2024
Merged via the queue into argumentcomputer:main with commit de92cfb Mar 2, 2024
11 checks passed
@gabriel-barrett gabriel-barrett deleted the lem-circuit-refactor branch March 2, 2024 03:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@porcuquine porcuquine porcuquine approved these changes

@arthurpaulino arthurpaulino arthurpaulino 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.
3 participants
@gabriel-barrett @arthurpaulino @porcuquine