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

Read cached output line-by-line #7737

Merged
merged 1 commit into from
Dec 22, 2019
Merged

Conversation

Mark-Simulacrum
Copy link
Member

This avoids loading potentially gigabytes of output into memory, which
can cause OOMs.

Fixes #7736.

This does not add a test as I don't really want to generate gigabytes of output (that seems like a bad idea) -- and it's unclear how to test other than by causing OOM on (most) CI systems, and it's unlikely that we'll actually regress here.

This avoids loading potentially gigabytes of output into memory, which
can cause OOMs.
@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 22, 2019
@ehuss
Copy link
Contributor

ehuss commented Dec 22, 2019

Thanks!
@bors r+

@bors
Copy link
Collaborator

bors commented Dec 22, 2019

📌 Commit 5e64197 has been approved by ehuss

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 22, 2019
@bors
Copy link
Collaborator

bors commented Dec 22, 2019

⌛ Testing commit 5e64197 with merge de4a5c9...

bors added a commit that referenced this pull request Dec 22, 2019
Read cached output line-by-line

This avoids loading potentially gigabytes of output into memory, which
can cause OOMs.

Fixes #7736.

This does not add a test as I don't really want to generate gigabytes of output (that seems like a bad idea) -- and it's unclear how to test other than by causing OOM on (most) CI systems, and it's unlikely that we'll actually regress here.
@bors
Copy link
Collaborator

bors commented Dec 22, 2019

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing de4a5c9 to master...

@bors bors merged commit 5e64197 into rust-lang:master Dec 22, 2019
bors added a commit that referenced this pull request Jan 22, 2020
Fix cache replay including extra newlines.

The compiler output cache replay was changed in #7737 to use `BufReader::read_line` instead of `str::lines`. `read_line`, unlike `lines`, includes the trailing line ending. The code is written assuming that the line endings are stripped, so make sure they are stripped here, too.

This only happens for non-JSON messages, like `RUSTC_LOG`.
@ehuss ehuss added this to the 1.42.0 milestone Feb 6, 2022
@Mark-Simulacrum Mark-Simulacrum deleted the cache-not-mem branch July 10, 2022 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

loading cached compiler messages OOMs system
5 participants