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

Inneficiency from extra frame #729

Closed
arthurpaulino opened this issue Oct 7, 2023 · 6 comments · Fixed by #887
Closed

Inneficiency from extra frame #729

arthurpaulino opened this issue Oct 7, 2023 · 6 comments · Fixed by #887
Labels
CO-Compilers Compilers track enhancement New feature or request

Comments

@arthurpaulino
Copy link
Contributor

arthurpaulino commented Oct 7, 2023

Suppose that rc == 1000 and our computation needed precisely 1000 frames. But our functions that generate frames are adding an extra padding (trivial, terminal -> terminal) frame at the end. Then we end up with 1001 frames.

That extra frame, alone, is making our pipeline generate other 999 similar frames so we can fill the other multiframe up and we end up with 2000 frames in total.

This behavior is being expected as the correct behavior in Prover::expected_total_iterations, which is used in tests.

@arthurpaulino arthurpaulino added enhancement New feature or request CO-Compilers Compilers track labels Oct 7, 2023
@arthurpaulino arthurpaulino self-assigned this Oct 7, 2023
@porcuquine
Copy link
Contributor

If I read this correctly, you're proposing to address this TODO.

@porcuquine
Copy link
Contributor

If so, please make sure to address the need for care mentioned there. This also suggests we may want to defer any such minor optimization — since it seems plausible that we explicitly drop support for SnarkPack+ (which would simplify making this change safely).

@arthurpaulino
Copy link
Contributor Author

Thanks a lot for enhancing the context of this issue!

@arthurpaulino
Copy link
Contributor Author

I'll defer this change. Thanks again 🙏🏼

@arthurpaulino arthurpaulino removed their assignment Oct 8, 2023
@johnchandlerburnham
Copy link
Contributor

Would be awesome if we could s/Dangerous/Inefficient/g and reserve "danger" for things which have semantics-breaking implications, which this, afaiu, does not

@arthurpaulino arthurpaulino changed the title Dangerous extra frame Inneficiency from extra frame Oct 9, 2023
@porcuquine
Copy link
Contributor

For clarity, it's not even really an inefficiency in the algorithmic sense. Once in rc times, a single extra fold may be required. This isn't significant in any normal use case. This is mainly an issue in the weird edge cases like our ivc sha256 benchmark, where for comparative purposes we're trying to avoid folding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CO-Compilers Compilers track enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants