-
Notifications
You must be signed in to change notification settings - Fork 25
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: switch ld
with lld
on linux, fixes OOM
#1388
Conversation
6dc3bd4
to
32de843
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1388 +/- ##
==========================================
- Coverage 74.18% 4.83% -69.36%
==========================================
Files 359 34 -325
Lines 36240 2109 -34131
Branches 36240 2109 -34131
==========================================
- Hits 26886 102 -26784
+ Misses 7220 1998 -5222
+ Partials 2134 9 -2125
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
bb6d35e
to
19321a8
Compare
Benchmark movements: full_committer_flow performance improved 😺 |
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.
Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @elintul)
.github/workflows/committer_ci.yml
line 90 at r1 (raw file):
# Benchmark the new code, splitting the benchmarks, and prepare the results for posting a comment. - uses: ./.github/actions/bootstrap
This calls dependencies.sh, which was missing (probably wasn't needed?).
Code quote:
- uses: ./.github/actions/bootstrap
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.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @elintul and @giladchase)
a discussion (no related file):
please bump blockifier and committer on py side and open PR to pass hodor
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.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @elintul and @giladchase)
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.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @giladchase)
We've recently taken to limit parallelism during builds due to Out Of Memory errors we got while running `cargo test` on 16-core machine, or 8-core machines running with `-j16`. We hit OOM due to two reasons: 1. Commit fae5aed introduced `llvm` dependencies to the blockifier, which notably increased compilation and linking time. 2. Before fae5aed, we already had a quite memory-heavy linking stage, specifically ~7-8 `ld` instances running in parallel, each with ~2-3GB of memory, which was just enough not to hit OOM on most 16core machines. This was primarily due to a combination of factors: 2.1 the blockifier being a large lib that needs a 2-3GB `ld` process to link in isolation. 2.2 many crates have the blockifier as a dependency (transitively). 2.3 `ld` naively parallelizes linking tasks. The combination of the above three issues caused a heavily parallelized environment to create many heavy linking jobs that shared no memory (blockifier linking was duplicated). This PR switches to the rust linker from `ld` to `lld`, thus solving the OOM issue without artificially bounding the amount of parallelization. `lld` handles parallelizing better and reduces memory stress from ~10 2-3GB `ld` linker instances post fae5aed, to ~2 1-2GB `lld` linker instances for the same tasks, which has a minimal memory impact. Moreover, `lld` is already the default linker on `rust` nightly, and will eventually become the default on stable (see code comment for link to tracking issue).
19321a8
to
bf80e0c
Compare
Benchmark movements: |
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @giladchase)
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.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @giladchase)
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.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @giladchase)
a discussion (no related file):
Previously, dorimedini-starkware wrote…
please bump blockifier and committer on py side and open PR to pass hodor
Done, python is only failing on a flaky test which isn't a blocker.
We've recently taken to limit parallelism during builds due to Out Of Memory errors we got while running `cargo test` on 16-core machine, or 8-core machines running with `-j16`. We hit OOM due to two reasons: 1. Commit fae5aed introduced `llvm` dependencies to the blockifier, which notably increased compilation and linking time. 2. Before fae5aed, we already had a quite memory-heavy linking stage, specifically ~7-8 `ld` instances running in parallel, each with ~2-3GB of memory, which was just enough not to hit OOM on most 16core machines. This was primarily due to a combination of factors: 2.1 the blockifier being a large lib that needs a 2-3GB `ld` process to link in isolation. 2.2 many crates have the blockifier as a dependency (transitively). 2.3 `ld` naively parallelizes linking tasks. The combination of the above three issues caused a heavily parallelized environment to create many heavy linking jobs that shared no memory (blockifier linking was duplicated). This PR switches to the rust linker from `ld` to `lld`, thus solving the OOM issue without artificially bounding the amount of parallelization. `lld` handles parallelizing better and reduces memory stress from ~10 2-3GB `ld` linker instances post fae5aed, to ~2 1-2GB `lld` linker instances for the same tasks, which has a minimal memory impact. Moreover, `lld` is already the default linker on `rust` nightly, and will eventually become the default on stable (see code comment for link to tracking issue). Co-Authored-By: Gilad Chase <[email protected]>
We've recently taken to limit parallelism during builds due to Out Of Memory errors we got while running
cargo test
on 16-core machine, or 8-core machines running with-j16
.We hit OOM due to two reasons:
llvm
dependencies to the blockifier, which notably increased compilation and linking time.ld
instances running in parallel, each with ~2-3GB of memory, which was just enough not to hit OOM on most 16core machines.This was primarily due to a combination of factors:
2.1 the blockifier being a large lib that needs a 2-3GB
ld
processto link in isolation.
2.2 many crates have the blockifier as a dependency (transitively).
2.3
ld
naively parallelizes linking tasks.The combination of the above three issues caused a heavily parallelized environment to create many heavy linking jobs that shared no memory (blockifier linking was duplicated).
Switching
lld
handles parallelizing better and reduces memory stress from ~10 2-3GBld
linker instances post fae5aed, to ~2 1-2GBlld
linker instances for the same tasks, which has a minimal memory impact.Moreover,
lld
is already the default linker onrust
nightly, and will eventually become the default on stable (see code comment for link to tracking issue).This change is