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

perf: buffer IO for Keystore (de)serialization #1760

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Oppen
Copy link
Contributor

@Oppen Oppen commented Apr 22, 2024

What ❔

Use BufReader and BufWriter rather than serializing to and
deserializing from memory, reducing one of the memory usage peaks caused
by temporaries.
For one particular case this avoids a 13GiB allocation.

Why ❔

The CPU prover experiences high memory usage that can bring down some instances.
This is an ongoing effort to reduce sailing and peak memory usage.

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted via zk fmt and zk lint.
  • Spellcheck has been run via zk spellcheck.
  • Linkcheck has been run via zk linkcheck.

@EmilLuta
Copy link
Contributor

I tried testing the PR, but that's not possible as we're in the middle of 1.5.0 upgrade. Will follow up once the upgrade is done.

@EmilLuta
Copy link
Contributor

P.S. There are a few unused variables and warning lints that can be addressed. I'd appreciate if we do that.

warning: unused imports: `Read`, `self`
 --> vk_setup_data_generator_server_fri/src/keystore.rs:2:10
  |
2 |     fs::{self, File},
  |          ^^^^
3 |     io::{BufReader, BufWriter, Read},
  |                                ^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

warning: variable does not need to be mutable
   --> vk_setup_data_generator_server_fri/src/keystore.rs:271:13
    |
271 |         let mut file = File::open(filepath.clone())
    |             ----^^^^
    |             |
    |             help: remove this `mut`
    |
    = note: `#[warn(unused_mut)]` on by default

@Oppen
Copy link
Contributor Author

Oppen commented Apr 25, 2024

I tried testing the PR, but that's not possible as we're in the middle of 1.5.0 upgrade. Will follow up once the upgrade is done.

If possible, comment when that's done if you need any update to the code. I check Github notifications so I'll see it.

Re: warnings: of course, I'll fix those soon.

@EmilLuta
Copy link
Contributor

EmilLuta commented May 3, 2024

We merged 1.5.0. Let's merge upstream and then we can test it.

Use `BufReader` and `BufWriter` rather than serializing to and
deserializing from memory, reducing one of the memory usage peaks caused
by temporaries.
For one particular case this avoids a 13GiB allocation.
@Oppen
Copy link
Contributor Author

Oppen commented May 4, 2024

Rebased and fixed the warnings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants