Null terminate core::panic::Location file strings
#117431
Closed
+102
−20
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@bors try @rust-timer queue |
|
r? @cuviper (rustbot has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
rustbot
added
S-waiting-on-review
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 31, 2023
Null terminate `core::panic::Location` file strings This shrinks `Location` by a usize, making it 24->16 bytes on x86-64. This reduces binary size. I quickly measure `cargo install ripgrep`, which was reduced by 0.3%. That's not a lot, but it's something and the code change is quite simple. From some quick measurements, I expect librustc_driver to shrink by more, but rustc-perf should reveal more. The change to const interning is necessary, see https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/unsupported.20untyped.20pointer.20in.20constant.20error/near/399446285
rust-cloud-vms
bot
force-pushed
the
nul-location
branch
from
ccff2ba to
4761094
Compare
|
Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
RalfJung
reviewed
Oct 31, 2023
| // For better errors later, mark the allocation as immutable. | ||
| alloc.mutability = Mutability::Not; | ||
| // The Location does have leftover allocations, but that's fine, as we control it. | ||
| if alloc_kind != MemoryKind::CallerLocation { |
There was a problem hiding this comment.
Uh, this is bad. That code is already terrible and my attempts at cleaning it up in #116745 are kind of stuck. I really don't like adding more complications here.
RalfJung
reviewed
Oct 31, 2023
| MemoryKind::CallerLocation, | ||
| Mutability::Not, | ||
| ) | ||
| .unwrap() | ||
| }; |
There was a problem hiding this comment.
I think you can avoid the interning change by instead manually interning the inner allocation with the string here.
There was a problem hiding this comment.
that sounds good. how exactly do i do the interning? is it the
let alloc = tcx.mk_const_alloc(alloc);
tcx.set_alloc_id_memory(alloc_id, alloc);part?
There was a problem hiding this comment.
Yeah that's the core thing. We have some helpers in interpret/intern.rs. Possible one of the existing one needs extension, currently there's basically 2 helpers for 2 users but it'd be good to start establishing some patterns. (That can also happen later though.)
oli-obk
reviewed
Oct 31, 2023
This comment has been minimized.
This comment has been minimized.
bjorn3
reviewed
Oct 31, 2023
| #[cfg(not(bootstrap))] | ||
| pub struct Location<'a> { | ||
| file: *const (), | ||
| _file_actual: PhantomData<&'a CStr>, |
There was a problem hiding this comment.
Why does Location have a lifetime? Isn't it always 'static?
There was a problem hiding this comment.
I don't know either - but it's stable :(
This saves 8 bytes in `Location` and allows for deduplicating file names more effectively through the linker in the future (by placing them in special seections).
rust-cloud-vms
bot
force-pushed
the
nul-location
branch
from
4761094 to
fc2f71f
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Nemo157
reviewed
Oct 31, 2023
| #[stable(feature = "panic_hooks", since = "1.10.0")] | ||
| #[cfg(not(bootstrap))] | ||
| pub struct Location<'a> { | ||
| file: *const (), |
There was a problem hiding this comment.
Needs an explicit Send/Sync impl now to keep the same API.
| unsafe { | ||
| let cstr = CStr::from_ptr(self.file as _); | ||
| let len = cstr.count_bytes(); | ||
| crate::str::from_utf8_unchecked(crate::slice::from_raw_parts(self.file as _, len)) |
There was a problem hiding this comment.
This makes calls here and to related (Hash, Debug, PartialEq, etc.) functions much more expensive. I guess we may not be able to do much better... but I think the binary size vs. possibly slower binaries that are keeping these locations around (e.g., HashMap) isn't as obvious.
It's possible we could make this near equivalent by avoiding the upfront 0-byte search internally.
There was a problem hiding this comment.
Another option would be doing a varint length prefix instead of a nul terminator. For short paths it would only be 1 byte so the same size as a nul terminator.
|
there are quite some improvements that can be done, but let's perf this first |
This comment has been minimized.
This comment has been minimized.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 31, 2023
Null terminate `core::panic::Location` file strings This shrinks `Location` by a usize, making it 24->16 bytes on x86-64. This reduces binary size. I quickly measure `cargo install ripgrep`, which was reduced by 0.3%. That's not a lot, but it's something and the code change is quite simple. From some quick measurements, I expect librustc_driver to shrink by more, but rustc-perf should give us more numbers. The change to const interning is necessary, see https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/unsupported.20untyped.20pointer.20in.20constant.20error/near/399446285
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (c5474df): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 636.003s -> 634.282s (-0.27%) |
rustbot
removed
the
S-waiting-on-perf
Are you intending to do more here? |
|
Given that this improvement here doesn't seem to really help, probably not. |
13 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This shrinks
Locationby a usize, making it 24->16 bytes on x86-64. This reduces binary size. I quickly measurecargo install ripgrep, which was reduced by 0.3%. That's not a lot, but it's something and the code change is quite simple. From some quick measurements, I expect librustc_driver to shrink by more, but rustc-perf should give us more numbers.The change to const interning is necessary, see https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/unsupported.20untyped.20pointer.20in.20constant.20error/near/399446285