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(pd): drop unused snapshots #2868

Merged
merged 2 commits into from
Jul 25, 2023
Merged

fix(pd): drop unused snapshots #2868

merged 2 commits into from
Jul 25, 2023

Conversation

conorsch
Copy link
Contributor

@conorsch conorsch commented Jul 25, 2023

Trying to resolve a memory leak in pd. These manual drops are a first pass, and appear to reduce memory consumption, but there's still a leak, according to bytehound. We'll continue to investigate.

Includes a missed to_proto that's a nice to have, but likely doesn't constitute a fix for our problem.

Refs #2867.

Used this script to generate loadtests against a local pd instance, for memory profiling efforts. Steps I used to test:

pd testnet unsafe-reset-all
pd testnet join https://rpc.testnet-preview.penumbra.zone
# start pd & tendermint, wait for genesis to complete
./deployments/scripts/pcli-client-test -n 1000 -p 100

Refs #2867.

@conorsch conorsch temporarily deployed to smoke-test July 25, 2023 18:43 — with GitHub Actions Inactive
Comment on lines 266 to 267
// Don't let the block hang around while we serve other clients.
std::mem::drop(block);
Copy link
Member

Choose a reason for hiding this comment

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

This isn't necessary, because the compact block doesn't have any shared references in it, and the explicit drop distracts from the case that is important (dropping the snapshot)

@hdevalence hdevalence temporarily deployed to smoke-test July 25, 2023 22:27 — with GitHub Actions Inactive
conorsch and others added 2 commits July 25, 2023 16:17
Used this script to generate loadtests against a local pd instance, for
memory profiling efforts. Steps I used to test:

  pd testnet unsafe-reset-all
  pd testnet join https://rpc.testnet-preview.penumbra.zone
  # start pd & tendermint, wait for genesis to complete
  ./deployments/scripts/pcli-client-test -n 1000 -p 100

Refs #2867.
Trying to resolve a memory leak in pd. These manual drops are a first
pass, and appear to reduce memory consumption, but there's still a leak,
according to bytehound. We'll continue to investigate.

Includes a missed `to_proto` that's a nice to have, but likely doesn't-
constitute a fix for our problem.

Refs #2867.

Co-Authored-By: Henry de Valence <hdevalence@penumbralabs.xyz>
@conorsch conorsch force-pushed the debug-pd-mem-consumption branch from 54a7c8e to ed6d38c Compare July 25, 2023 23:26
@conorsch conorsch temporarily deployed to smoke-test July 25, 2023 23:26 — with GitHub Actions Inactive
@conorsch conorsch marked this pull request as ready for review July 25, 2023 23:28
@conorsch
Copy link
Contributor Author

Cleaned up the commit history. After CI runs, I'm going to merge this, then cherry-pick the pd changes onto a release branch and tag as v0.56.1. As such, refs #2761. We still plan to release Testnet 57 tomorrow (#2827).

@conorsch conorsch changed the title test(pd): add client test script fix(pd): drop unused snapshots Jul 25, 2023
@conorsch conorsch merged commit 4b7c386 into main Jul 25, 2023
@conorsch conorsch deleted the debug-pd-mem-consumption branch July 25, 2023 23:49
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.

2 participants