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

Box DiagnosticBuilder. #64374

Merged
merged 1 commit into from
Sep 14, 2019
Merged

Conversation

nnethercote
Copy link
Contributor

It's a large type -- 176 bytes on 64-bit. And it's passed around and
returned from a lot of functions, including within PResult.

This commit boxes it, which reduces memory traffic. In particular,
PResult shrinks to 16 bytes in the best case; this reduces instruction
counts by up to 2% on various workloads. The commit touches a lot of
lines but it's almost all trivial plumbing changes.

@rust-highfive
Copy link
Collaborator

r? @zackmdavis

(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 Sep 11, 2019
@nnethercote
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented Sep 11, 2019

⌛ Trying commit 176bc8cb4c66adfbd2c4220ecf2f8718119e6ae1 with merge 10e8cb29dd1ac7824382d27c5b6e62eedaa42431...

@Centril
Copy link
Contributor

Centril commented Sep 11, 2019

Is there a process by which you selected these specific cases to do the boxing?
Would it be detrimental to apply this change inside the DiagnosticBuilder type instead?

@nnethercote nnethercote mentioned this pull request Sep 11, 2019
@nnethercote
Copy link
Contributor Author

My strategy was "gotta box 'em all", because there's not much point only boxing some, and if you did, there isn't an obvious dividing line between boxed and non-boxed.

Boxing within DiagnosticBuilder would be possible. It would make DiagnosticBuilder itself a bit more complicated, but would avoid the sweeping DiagnosticBuilder to Box<DiagnosticBuilder> changes in the current commit.

@Centril
Copy link
Contributor

Centril commented Sep 11, 2019

Boxing within DiagnosticBuilder would be possible. It would make DiagnosticBuilder itself a bit more complicated, but would avoid the sweeping DiagnosticBuilder to Box<DiagnosticBuilder> changes in the current commit.

The main reason I would prefer to do an internal change is to make the developer UX better so that that "doing the wrong thing" is not possible here. I think I'd personally forget about your PR after a while and forget to do the manual boxing so not having to think about it would be neat.

cc also @estebank

@petrochenkov
Copy link
Contributor

Why is it memcpy'ed in the first place?
It looks large enough to be passed and returned by pointer in ABI.

Is it something that is supposed to be addressed by RVO, which rustc doesn't always do when it should right now?
If that's the case, then I'd prefer to make the box internal so it could be removed without touching all the users when RVO is fixed.

@rust-highfive
Copy link
Collaborator

The job mingw-check of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-09-11T10:45:58.4130118Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-09-11T10:45:59.1803827Z ##[command]git config gc.auto 0
2019-09-11T10:45:59.1807473Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-09-11T10:45:59.1809463Z ##[command]git config --get-all http.proxy
2019-09-11T10:45:59.1813653Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/64374/merge:refs/remotes/pull/64374/merge
---
2019-09-11T10:57:50.5276168Z configure: build.locked-deps    := True
2019-09-11T10:57:50.5276383Z configure: llvm.ccache          := sccache
2019-09-11T10:57:50.5276860Z configure: build.cargo-native-static := True
2019-09-11T10:57:50.5277275Z configure: dist.missing-tools   := True
2019-09-11T10:57:50.5277762Z configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
2019-09-11T10:57:50.5279622Z configure: writing `config.toml` in current directory
2019-09-11T10:57:50.5279955Z configure: 
2019-09-11T10:57:50.5280630Z configure: run `python /checkout/x.py --help`
2019-09-11T10:57:50.5280946Z configure: 
---
2019-09-11T11:03:17.5879091Z == clock drift check ==
2019-09-11T11:03:17.5901063Z   local time: Wed Sep 11 11:03:17 UTC 2019
2019-09-11T11:03:17.7394505Z   network time: Wed, 11 Sep 2019 11:03:17 GMT
2019-09-11T11:03:17.7398405Z == end clock drift check ==
2019-09-11T11:03:18.8417134Z ##[error]Bash exited with code '1'.
2019-09-11T11:03:18.8500637Z ##[section]Starting: Checkout
2019-09-11T11:03:18.8503320Z ==============================================================================
2019-09-11T11:03:18.8503401Z Task         : Get sources
2019-09-11T11:03:18.8503448Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Mark-Simulacrum
Copy link
Member

Indeed, and I would actually look at boxing the Diagnostic internally instead, or maybe just reducing it's size via some other optimizations.

@bors
Copy link
Contributor

bors commented Sep 11, 2019

☀️ Try build successful - checks-azure
Build commit: 10e8cb29dd1ac7824382d27c5b6e62eedaa42431

It's a large type -- 176 bytes on 64-bit. And it's passed around and
returned from a lot of functions, including within PResult.

This commit boxes it, which reduces memory traffic. In particular,
`PResult` shrinks to 16 bytes in the best case; this reduces instruction
counts by up to 2% on various workloads.
@nnethercote
Copy link
Contributor Author

nnethercote commented Sep 11, 2019

I have updated the code to use an internal box. It made the patch much less invasive -- thanks for the suggestion, @Centril.

I don't know why RVO isn't being applied. I just go where the profiler tells me to, and it told me that lots of memcpys were happening because of PResult. I have added a comment about this.

I also fixed the mingw failure.

@bors try

@bors
Copy link
Contributor

bors commented Sep 11, 2019

⌛ Trying commit 2fcd870 with merge 25dba6526006eb8d4c2da901bcf74f00b4ed6383...

@bors
Copy link
Contributor

bors commented Sep 12, 2019

⌛ Trying commit 2fcd870 with merge f81c392...

bors added a commit that referenced this pull request Sep 12, 2019
Box `DiagnosticBuilder`.

It's a large type -- 176 bytes on 64-bit. And it's passed around and
returned from a lot of functions, including within `PResult`.

This commit boxes it, which reduces memory traffic. In particular,
`PResult` shrinks to 16 bytes in the best case; this reduces instruction
counts by up to 2% on various workloads. The commit touches a lot of
lines but it's almost all trivial plumbing changes.
@bors
Copy link
Contributor

bors commented Sep 12, 2019

☀️ Try build successful - checks-azure
Build commit: f81c392

@nnethercote
Copy link
Contributor Author

@rust-timer build f81c392

@rust-timer
Copy link
Collaborator

Success: Queued f81c392 with parent f0b58fc, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit f81c392, comparison URL.

@nnethercote
Copy link
Contributor Author

The perf results look good, reducing instruction counts by up to 2.6%.

@zackmdavis
Copy link
Member

Great! 💖

@bors r+

@bors
Copy link
Contributor

bors commented Sep 14, 2019

📌 Commit 2fcd870 has been approved by zackmdavis

@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 Sep 14, 2019
Centril added a commit to Centril/rust that referenced this pull request Sep 14, 2019
…r=zackmdavis

Box `DiagnosticBuilder`.

It's a large type -- 176 bytes on 64-bit. And it's passed around and
returned from a lot of functions, including within `PResult`.

This commit boxes it, which reduces memory traffic. In particular,
`PResult` shrinks to 16 bytes in the best case; this reduces instruction
counts by up to 2% on various workloads. The commit touches a lot of
lines but it's almost all trivial plumbing changes.
Centril added a commit to Centril/rust that referenced this pull request Sep 14, 2019
…r=zackmdavis

Box `DiagnosticBuilder`.

It's a large type -- 176 bytes on 64-bit. And it's passed around and
returned from a lot of functions, including within `PResult`.

This commit boxes it, which reduces memory traffic. In particular,
`PResult` shrinks to 16 bytes in the best case; this reduces instruction
counts by up to 2% on various workloads. The commit touches a lot of
lines but it's almost all trivial plumbing changes.
Centril added a commit to Centril/rust that referenced this pull request Sep 14, 2019
…r=zackmdavis

Box `DiagnosticBuilder`.

It's a large type -- 176 bytes on 64-bit. And it's passed around and
returned from a lot of functions, including within `PResult`.

This commit boxes it, which reduces memory traffic. In particular,
`PResult` shrinks to 16 bytes in the best case; this reduces instruction
counts by up to 2% on various workloads. The commit touches a lot of
lines but it's almost all trivial plumbing changes.
bors added a commit that referenced this pull request Sep 14, 2019
Rollup of 17 pull requests

Successful merges:

 - #63846 (Added table containing the system calls used by Instant and SystemTime.)
 - #64116 (Fix minor typo in docs.)
 - #64203 (A few cosmetic improvements to code & comments in liballoc and libcore)
 - #64302 (Shrink `ObligationCauseCode`)
 - #64372 (use randSecure and randABytes)
 - #64374 (Box `DiagnosticBuilder`.)
 - #64375 (Fast path for vec.clear/truncate )
 - #64378 (Fix inconsistent link formatting.)
 - #64384 (Trim rustc-workspace-hack)
 - #64393 ( declare EnvKey before use to fix build error)
 - #64420 (Inline `mark_neighbours_as_waiting_from`.)
 - #64422 (Remove raw string literal quotes from error index descriptions)
 - #64423 (Add self to .mailmap)
 - #64425 (typo fix)
 - #64431 (fn ptr is structural match)
 - #64435 (codegen: use "_N" (like for other locals) instead of "argN", for argument names.)
 - #64439 (fix #64430, confusing `owned_box` error message in no_std build)

Failed merges:

r? @ghost
@bors bors merged commit 2fcd870 into rust-lang:master Sep 14, 2019
@nnethercote nnethercote deleted the box-DiagnosticBuilder branch September 14, 2019 23:27
@nnethercote nnethercote removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 16, 2019
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.

8 participants