-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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(static-files): break Arc
reference cycle
#6795
Conversation
bf83f5a
to
50e4e4f
Compare
Arc
reference cycle
Arc
reference cycleArc
reference cycle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems fine
let start = Instant::now(); | ||
|
||
let static_file_provider = StaticFileProvider(reader.upgrade().unwrap()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs .expects
and a note that this is fine because this is only accessible as a RefMut, so the owned type always exists,
in other words it's not possible to detach the StaticFileProviderRW from the StaticFileProvider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
.github/workflows/unit.yml
Outdated
@@ -1,21 +1,17 @@ | |||
# Runs unit tests. | |||
|
|||
name: unit | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lsp formatting, reverted
} | ||
// Drop the provider without committing to the database. | ||
drop(provider); | ||
// TODO: replace with `tempdir` usage, so the temp directory is removed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
24b528b
to
1d68d32
Compare
a729f10
to
7249fff
Compare
15d6d69
to
181883c
Compare
Ethereum State Tests were failing on
st_bad_opcode
andst_time_consuming
because of CI OOMs caused by memory leaks due toStaticFileProvider
andStaticFileProviderRW
being referenced by each other.We fix that by making a reference from
StaticFileProviderRW
toStaticFileProvider
weak, see https://doc.rust-lang.org/std/sync/struct.Arc.html#breaking-cycles-with-weak.reth/crates/storage/provider/src/providers/static_file/writer.rs
Lines 28 to 32 in 0a5a9bd
reth/crates/storage/provider/src/providers/static_file/manager.rs
Lines 68 to 71 in 0a5a9bd
reth/crates/storage/provider/src/providers/static_file/manager.rs
Lines 45 to 47 in 0a5a9bd
Additionally, I added parallelism to Ethereum State Tests. Some tests have up to 20k cases, so we can leverage parallel runs here.
It should be fine memory-wise, because currently tests take 600MB max, and our CI machine has 4 cores and 16GB RAM.
Valgrind also successfully detects a memory leak on a minimal repro: