Skip to content

Buffer LLVM's object output stream #53524

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
merged 1 commit into from
Aug 22, 2018
Merged

Conversation

alexcrichton
Copy link
Member

In some profiling on OSX I saw the write syscall as quite high up on
the profiling graph, which is definitely not good! It looks like we're
setting the output stream of an object file as directly to a file
descriptor which means that we run the risk of doing lots of little
writes rather than a few large writes.

This commit fixes this issue by adding a buffered stream on the output,
causing the write syscall to disappear from the profiles on OSX.

In some profiling on OSX I saw the `write` syscall as quite high up on
the profiling graph, which is definitely not good! It looks like we're
setting the output stream of an object file as directly to a file
descriptor which means that we run the risk of doing lots of little
writes rather than a few large writes.

This commit fixes this issue by adding a buffered stream on the output,
causing the `write` syscall to disappear from the profiles on OSX.
@rust-highfive
Copy link
Contributor

r? @aturon

(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 Aug 20, 2018
@alexcrichton
Copy link
Member Author

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned aturon Aug 20, 2018
@@ -556,7 +556,8 @@ LLVMRustWriteOutputFile(LLVMTargetMachineRef Target, LLVMPassManagerRef PMR,
}

#if LLVM_VERSION_GE(7, 0)
unwrap(Target)->addPassesToEmitFile(*PM, OS, nullptr, FileType, false);
buffer_ostream BOS(OS);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason this is only done on 7.0+? Seems like we could do it for other LLVMs as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to muck around with older versions without testing, so I opted to leave it here (it doesn't really matter for older llvm, a slightly slower compiler is the least of those worries)

@eddyb
Copy link
Member

eddyb commented Aug 20, 2018

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 20, 2018

📌 Commit 9d54bf8 has been approved by eddyb

@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 Aug 20, 2018
@bors
Copy link
Collaborator

bors commented Aug 22, 2018

⌛ Testing commit 9d54bf8 with merge 329dde5...

bors added a commit that referenced this pull request Aug 22, 2018
Buffer LLVM's object output stream

In some profiling on OSX I saw the `write` syscall as quite high up on
the profiling graph, which is definitely not good! It looks like we're
setting the output stream of an object file as directly to a file
descriptor which means that we run the risk of doing lots of little
writes rather than a few large writes.

This commit fixes this issue by adding a buffered stream on the output,
causing the `write` syscall to disappear from the profiles on OSX.
@bors
Copy link
Collaborator

bors commented Aug 22, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 329dde5 to master...

@bors bors merged commit 9d54bf8 into rust-lang:master Aug 22, 2018
@alexcrichton alexcrichton deleted the buffer-out branch August 29, 2018 16:33
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.

6 participants