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

chore: Improve and update dependencies #104

Merged
merged 5 commits into from
Jun 24, 2024
Merged

chore: Improve and update dependencies #104

merged 5 commits into from
Jun 24, 2024

Conversation

huitseeker
Copy link
Member

@huitseeker huitseeker commented Jun 24, 2024

  • Replaced standard HashMap with hashbrown's HashMap in execute.rs for optimization.
  • Modified prove method in StarkMachine to take a new additional argument opts of type SphinxCoreOpts.
  • Updated several dependencies branch references in Cargo.toml including changing sphinx-core to use plonk-rebased branch.

Companion PR of argumentcomputer/sphinx#72

Note

CI failure is expected, CI should be re-run once argumentcomputer/sphinx#72 is merged.

- Replaced standard HashMap with hashbrown's HashMap in `execute.rs` for optimization.
- Modified `prove` method in `StarkMachine` to take a new additional argument `opts` of type `SphinxCoreOpts`.
- Updated several dependencies branch references in `Cargo.toml` including changing `sphinx-core` to use `plonk-rebased` branch.

Companion PR of argumentcomputer/sphinx#72
adr1anh
adr1anh previously approved these changes Jun 24, 2024
@arthurpaulino
Copy link
Member

arthurpaulino commented Jun 24, 2024

Replaced standard HashMap with hashbrown's HashMap in execute.rs for optimization.

Did you run the benchmarks? I recall not seeing any improvements from using this lib on lurk-rs. In fact, I remember seeing worse numbers.

arthurpaulino
arthurpaulino previously approved these changes Jun 24, 2024
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.

Okay, HashMap in execute.rs is not used in any perf-sensitive part of the code

@arthurpaulino arthurpaulino dismissed stale reviews from adr1anh and themself via c779d43 June 24, 2024 10:26
wwared
wwared previously approved these changes Jun 24, 2024
Copy link
Member

@wwared wwared left a comment

Choose a reason for hiding this comment

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

This is fine but CI is failing due to an outdated token, we just need to wait a bit for @samuelburnham

@huitseeker
Copy link
Member Author

@arthurpaulino this use of the hashbrown::HashMap is not elective, but due to interaction with the sphinx APIs which do in fact use it over the sdt::collections::hashmap.

@arthurpaulino arthurpaulino merged commit 24348a6 into main Jun 24, 2024
2 checks passed
@arthurpaulino arthurpaulino deleted the switch_plonk branch June 24, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants