Skip to content

Try to avoid fuzz disk fullness #3916

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

Merged

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Jul 8, 2025

No description provided.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jul 8, 2025

👋 Thanks for assigning @joostjager as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@tnull tnull requested a review from joostjager July 8, 2025 11:21
@tnull tnull force-pushed the 2025-07-avoid-fuzz-disk-fullness branch from 193e1bf to cd91843 Compare July 8, 2025 11:28
@tnull tnull changed the title Try to avvoid fuzz disk fullness Try to avoid fuzz disk fullness Jul 8, 2025
@tnull tnull force-pushed the 2025-07-avoid-fuzz-disk-fullness branch from cd91843 to 9fe30c8 Compare July 8, 2025 11:53
# Because we're fuzzing relatively few iterations, the maximum possible
# compiler optimizations aren't necessary, so switch to defaults.
sed -i 's/lto = true//' Cargo.toml
sed -i 's/codegen-units = 1//' Cargo.toml
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removing this is likely to slow the build step down substantially.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing this is likely to slow the build step down substantially.

Which part do you mean exactly. As mentioned below, lto was never used as it conflicts with codegen-units = 1.

With the sed commands above, we would fallback to defaults in CI, meaning lto = false (see https://doc.rust-lang.org/cargo/reference/profiles.html#dev), and codegen-units = 16. Do we expect the latter to make that much of an impact?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, the build step takes a minute, and making it single-threaded is unlikely to help :). It may be okay because we can parallelize across binaries, but no reason to slow it down more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I guess we'll have the benchmark for comparison in a bit. Let's see how long it will take.

Copy link
Contributor Author

@tnull tnull Jul 9, 2025

Choose a reason for hiding this comment

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

The fuzz job on this branch finished in ~4h 20m. Looking at recent examples of PRs where the CI used to pass, it did so in ~4h 15m. I think 5 min/2% difference is acceptable?

Copy link
Contributor Author

@tnull tnull Jul 9, 2025

Choose a reason for hiding this comment

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

Ah, 4h 13m this time around, so we even have some speedup compared to some recent PRs I looked at. Seems the variance in CI is larger than the impact of the actual change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright, yea, then the cross-binary parallelization dominates. Cool. Its less clear to me if this makes a difference on disk space usage, but it doesn't really matter.

@@ -18,13 +18,10 @@ export RUSTFLAGS="--cfg=secp256k1_fuzz --cfg=hashes_fuzz"
mkdir -p hfuzz_workspace/full_stack_target/input
pushd write-seeds
RUSTFLAGS="$RUSTFLAGS --cfg=fuzzing" cargo run ../hfuzz_workspace/full_stack_target/input
cargo clean
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we really not add a du in here to see what needs cleaning and what doesn't? Also to not unnecessarily increase build times.

And can the disk space in github be increased?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't we really not add a du in here to see what needs cleaning and what doesn't? Also to not unnecessarily increase build times.

Huh, we only clean up here and below once were done? So should not have ~any overhead to clean target dirs that you aren't using anymore?

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought it would lead to re-downloads of packages. Not that would matter much even if it was the case with a 4h job...

tnull added 2 commits July 9, 2025 09:13
.. to hopefully clear up some disk space in the process.
.. which marginally (~2%) increases the runtime while decreasing the
disk usage.
@tnull tnull force-pushed the 2025-07-avoid-fuzz-disk-fullness branch from 9fe30c8 to f6238fc Compare July 9, 2025 07:19
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Well, it passed, so it works well enough. Just gonna land this as trivial and we can do whatever it takes later to improve things if it breaks again.

@TheBlueMatt TheBlueMatt merged commit 42085b9 into lightningdevkit:main Jul 9, 2025
28 checks passed
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