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.

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

Fix mock prover performance regression for lookup arguments #445

Merged
merged 4 commits into from
Apr 19, 2022

Conversation

daira
Copy link
Contributor

@daira daira commented Dec 28, 2021

closes #398

Signed-off-by: Daira Hopwood daira@jacaranda.org

@daira daira force-pushed the mockprover-regression branch 2 times, most recently from 67611fd to fdd9362 Compare December 28, 2021 00:46
@codecov-commenter
Copy link

codecov-commenter commented Dec 28, 2021

Codecov Report

Merging #445 (fdd9362) into main (af8e0d6) will increase coverage by 0.09%.
The diff coverage is 71.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #445      +/-   ##
==========================================
+ Coverage   70.49%   70.58%   +0.09%     
==========================================
  Files          42       42              
  Lines        5297     5321      +24     
==========================================
+ Hits         3734     3756      +22     
- Misses       1563     1565       +2     
Impacted Files Coverage Δ
src/dev.rs 68.14% <71.62%> (+1.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af8e0d6...fdd9362. Read the comment docs.

@daira
Copy link
Contributor Author

daira commented Dec 28, 2021

I recommend to review ignoring whitespace changes.

ebfull
ebfull previously requested changes Dec 28, 2021
@str4d str4d added this to the Core Sprint 2021-50 milestone Dec 29, 2021
@str4d str4d added A-dev-tooling Area: Developer tooling I-performance labels Dec 29, 2021
@daira daira requested a review from ebfull December 29, 2021 17:09
@r3ld3v r3ld3v modified the milestones: Core Sprint 2021-50, 2021-52 Jan 3, 2022
@str4d
Copy link
Contributor

str4d commented Jan 4, 2022

This will conflict with #433; I'd prefer to merge that PR before this one, in part because it will simplify the diff here.

Copy link
Collaborator

@therealyingtong therealyingtong left a comment

Choose a reason for hiding this comment

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

utACK with comments.

@daira
Copy link
Contributor Author

daira commented Jan 13, 2022

Changing this back to draft because I need to change it to optimize fill rows rather than zero rows (as well as resolving conflicts).

@daira daira self-assigned this Jan 13, 2022
@daira daira marked this pull request as draft January 13, 2022 16:07
@daira daira force-pushed the mockprover-regression branch from 968b6e0 to 9b8ecc7 Compare February 14, 2022 23:53
are contained in the table, fixing a performance regression.
This includes an optimization for "fill rows", which are
assumed in this commit to be all-zeros.

closes zcash#398

Signed-off-by: Daira Hopwood <daira@jacaranda.org>
@daira daira force-pushed the mockprover-regression branch from 9b8ecc7 to 7107b83 Compare February 14, 2022 23:54
… row.

Signed-off-by: Daira Hopwood <daira@jacaranda.org>
@daira daira marked this pull request as ready for review February 15, 2022 00:35
Copy link
Contributor Author

@daira daira left a comment

Choose a reason for hiding this comment

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

Minor simplification.

@daira
Copy link
Contributor Author

daira commented Apr 12, 2022

This is still up-to-date and needs review. (@therealyingtong's utACK was before it changed substantially.)

@daira daira requested a review from therealyingtong April 12, 2022 17:53
@str4d str4d added this to the Release 5.0.0 milestone Apr 18, 2022
Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

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

ACK; I observe the desired speed-up.

@daira daira requested a review from str4d April 19, 2022 11:36
Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

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

re-utACK b48b032

@str4d str4d merged commit 606afb8 into zcash:main Apr 19, 2022
@daira daira deleted the mockprover-regression branch April 19, 2022 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[MockProver] Testing is slower after #389
6 participants