Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upstruct field reordering and optimization #37429
Conversation
rust-highfive
assigned
eddyb
Oct 27, 2016
eddyb
requested changes
Oct 27, 2016
| } | ||
|
|
||
| /// Extend the Struct with more fields. | ||
| pub fn extend<I>(&mut self, dl: &TargetDataLayout, | ||
| fn fill_in_fields<I>(&mut self, dl: &TargetDataLayout, |
This comment has been minimized.
This comment has been minimized.
eddyb
Oct 27, 2016
Member
You can probably move the whole body of fill_in_fields into fn new. Also maybe rename it to from_fields.
EDIT: Oh is it because of if fields.len() == 0 {return Ok(())}; below?
This comment has been minimized.
This comment has been minimized.
camlorn
Oct 27, 2016
Author
Contributor
I didn't think of it that way, but you're right in that that's a good reason. I just preferred to separate the overview from the steps, if you will.
This comment has been minimized.
This comment has been minimized.
eddyb
Nov 16, 2016
Member
The problem is that this function shouldn't be possible to call at all, this is really outlining, and there's no usefulness gained by having self - that is, I'd expect the Struct be created at the very end.
| // We have to fix the last element of path here as only we know the right value. | ||
| let mut i = *path.last().unwrap(); | ||
| i = st.field_index[i as usize]; | ||
| *path.last_mut().unwrap() = i; |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
camlorn
Oct 27, 2016
Author
Contributor
That wouldn't be an improvement because the borrow lasts too long. It would have to be wrapped in braces and indented further or I'd have to use mem::forget. We can't push the 0 onto the Vec while i exists.
This comment has been minimized.
This comment has been minimized.
eddyb
Oct 27, 2016
Member
Oh whoops. Only braces would work, btw, borrows can't even be terminated manually.
| let mut fields = fields.into_iter().map(|field| { | ||
| field.layout(infcx) | ||
| }).collect::<Vec<_>>(); | ||
| fields.insert(0, Ok(&discr)); |
This comment has been minimized.
This comment has been minimized.
eddyb
Oct 27, 2016
Member
None of these changes seem to be really needed - or are you planning to move the ZSTs here?
This comment has been minimized.
This comment has been minimized.
camlorn
Oct 27, 2016
Author
Contributor
This is because we can no longer iterate over the fields in order until after we have the struct.
| @@ -511,41 +512,70 @@ pub struct Struct { | |||
| /// If true, the size is exact, otherwise it's only a lower bound. | |||
| pub sized: bool, | |||
|
|
|||
| /// Offsets for the first byte of each field. | |||
| /// Offsets for the first byte of each field, ordered in increasing order. | |||
| /// The ith element of this vector is not always the ith field of the struct. | |||
| /// FIXME(eddyb) use small vector optimization for the common case. | |||
| pub offsets: Vec<Size>, | |||
This comment has been minimized.
This comment has been minimized.
eddyb
Oct 27, 2016
Member
To have the increasing order offsets (instead of declaration order - is it really necessary?) you need to rename the field to permuted_offsets or ordered_offsets and (re-)add a field_offset method.
This comment has been minimized.
This comment has been minimized.
camlorn
Oct 27, 2016
Author
Contributor
Should I just make it private and have field_offset and num_fields functions?
This comment has been minimized.
This comment has been minimized.
eddyb
Oct 27, 2016
Member
That sounds okay - but is it really necessary to have the offsets permuted?
camlorn
force-pushed the
camlorn:univariant_layout_optimization
branch
from
472a158
to
9553ddd
Oct 27, 2016
This comment has been minimized.
This comment has been minimized.
|
Sub. I'm sure we're gonna see some interesting stuff in the crater run, hopefully all fixable with a #[repr(C)] |
This comment has been minimized.
This comment has been minimized.
|
@arthurprs I'm waiting on #36622 at the moment. Then we shall see. Probably that can be worked around, but I don't have enough experience yet to know what I should or shouldn't be logging and also there's another project I can work on that's not Rust. If you see places that still assume fields are in order, tell me. I'm more than willing to accept help finding them. |
camlorn
force-pushed the
camlorn:univariant_layout_optimization
branch
from
9553ddd
to
af058d6
Nov 7, 2016
This comment has been minimized.
This comment has been minimized.
|
ping @eddyb Looks like some updates have happened since you last looked, want to take another peek? |
alexcrichton
added
T-compiler
relnotes
labels
Nov 10, 2016
This comment has been minimized.
This comment has been minimized.
|
also tagging with |
This comment has been minimized.
This comment has been minimized.
|
@alexcrichton |
This comment has been minimized.
This comment has been minimized.
|
Ah ok, no worries! Just checking up |
This comment has been minimized.
This comment has been minimized.
|
All but 18 of the Debuginfo still needs conversion. |
This comment has been minimized.
This comment has been minimized.
|
Some thoughts (long term, no immediate action required): It would be good to have two things related to field reordering:
Also, reordering accidentally uses stable sorting algorithm now, just because sorting in libstd is stable. |
This comment has been minimized.
This comment has been minimized.
|
@petrochenkov I like the idea of Both of these should be almost absurdly trivial to actually add. I could work out how to do it without leaning on @eddyb, I think. |
camlorn
force-pushed the
camlorn:univariant_layout_optimization
branch
from
376ac0c
to
408bed9
Nov 12, 2016
This comment has been minimized.
This comment has been minimized.
|
|
camlorn
force-pushed the
camlorn:univariant_layout_optimization
branch
from
408bed9
to
a565851
Nov 16, 2016
eddyb
reviewed
Nov 16, 2016
| } | ||
|
|
||
| /// Extend the Struct with more fields. | ||
| pub fn extend<I>(&mut self, dl: &TargetDataLayout, | ||
| fn fill_in_fields<I>(&mut self, dl: &TargetDataLayout, |
This comment has been minimized.
This comment has been minimized.
eddyb
Nov 16, 2016
Member
The problem is that this function shouldn't be possible to call at all, this is really outlining, and there's no usefulness gained by having self - that is, I'd expect the Struct be created at the very end.
| -> Result<(), LayoutError<'gcx>> | ||
| where I: Iterator<Item=Result<&'a Layout, LayoutError<'gcx>>> { | ||
| self.offsets.reserve(fields.size_hint().0); | ||
| let fields = fields.collect::<Result<Vec<_>, LayoutError<'gcx>>>()?; | ||
| if fields.len() == 0 {return Ok(())}; |
This comment has been minimized.
This comment has been minimized.
|
|
||
| self.offsets = vec![Size::from_bytes(0); fields.len()]; | ||
| let mut inverse_gep_index: Vec<u32> = Vec::with_capacity(fields.len()); | ||
| inverse_gep_index.extend(0..fields.len() as u32); |
This comment has been minimized.
This comment has been minimized.
eddyb
Nov 16, 2016
Member
This can be written as let mut inverse_gep_index: Vec<u32> = (0..fields.len() as u32).collect();
This comment has been minimized.
This comment has been minimized.
| inverse_gep_index.extend(0..fields.len() as u32); | ||
|
|
||
| if repr == attr::ReprAny { | ||
| let start: usize = if is_enum_variant {1} else {0}; |
This comment has been minimized.
This comment has been minimized.
| @@ -511,41 +512,76 @@ pub struct Struct { | |||
| /// If true, the size is exact, otherwise it's only a lower bound. | |||
| pub sized: bool, | |||
|
|
|||
| /// Offsets for the first byte of each field. | |||
| /// Offsets for the first byte of each field, ordered to match the tys. | |||
This comment has been minimized.
This comment has been minimized.
eddyb
Nov 16, 2016
Member
Could you refer to "the definition" or "definition order" instead of "types"? That is, the order the fields come in is the source definition order, the types just happen to be associated with each field so the most straight-forward way to get them is in source definition order, but they're secondary to the order.
| /// FIXME(eddyb) use small vector optimization for the common case. | ||
| pub offsets: Vec<Size>, | ||
|
|
||
| /// Maps field indices to GEP indices, depending how fields were permuted. |
This comment has been minimized.
This comment has been minimized.
eddyb
Nov 16, 2016
Member
"Maps from definition order to the order the fields were laid out in", maybe? Still not perfect though.
GEP should really not be mentioned if avoided. def_to_laid_order might work, although jarring.
This comment has been minimized.
This comment has been minimized.
camlorn
Nov 20, 2016
Author
Contributor
I ended up calling these source order and memory order, I don't think there's anything better. The gep_index vec is now memory_index.
If there's better nomenclature, I don't know what it is.
| }).collect::<Vec<_>>(); | ||
| fields.insert(0, Ok(&discr)); | ||
| let st = Struct::new(dl, | ||
| fields.iter().cloned(), |
This comment has been minimized.
This comment has been minimized.
eddyb
Nov 16, 2016
Member
What if you make Struct::new take the Vec<Layout>? Maybe that way the double allocating can be avoided.
eddyb
reviewed
Nov 16, 2016
| @@ -26,8 +26,9 @@ fn main() { | |||
| assert_eq!(size_of::<E>(), 1); | |||
| assert_eq!(size_of::<Option<E>>(), 1); | |||
| assert_eq!(size_of::<Result<E, ()>>(), 1); | |||
| assert_eq!(size_of::<S>(), 4); | |||
| assert_eq!(size_of::<Option<S>>(), 4); | |||
| // The next asserts are correct given the currently dumb field reordering algorithm, which actually makes this struct larger. | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
To anyone watching, all the run-pass tests now pass. |
camlorn
force-pushed the
camlorn:univariant_layout_optimization
branch
from
1fbefa0
to
825ec31
Nov 20, 2016
eddyb
reviewed
Nov 21, 2016
| @@ -1305,6 +1313,8 @@ impl<'tcx> EnumMemberDescriptionFactory<'tcx> { | |||
|
|
|||
| // Creates MemberDescriptions for the fields of a single enum variant. | |||
| struct VariantMemberDescriptionFactory<'tcx> { | |||
| // Cloned from the layout::Struct describing the variant. | |||
| offsets: Vec<layout::Size>, | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
camlorn
force-pushed the
camlorn:univariant_layout_optimization
branch
3 times, most recently
from
9bf17ce
to
b0c6c53
Nov 21, 2016
This comment has been minimized.
This comment has been minimized.
|
Crater report has 3 legitimate regressions, one of them a LLVM assertion, the others are a bug with packed: pub const SIZEOF_QUERY: usize = 21;
#[repr(C,packed)]
pub struct p0f_api_query {
pub magic: u32,
pub addr_type: u8,
pub addr: [u8; 16],
}The correct size is indeed 21, but this PR changes it to 24, presumably some logic went missing. |
This comment has been minimized.
This comment has been minimized.
|
Will there be another repr attribute? Something like |
eddyb
reviewed
Nov 23, 2016
| } | ||
| } | ||
|
|
||
| let at_least = if let Some(i) = min_from_extern { |
This comment has been minimized.
This comment has been minimized.
eddyb
reviewed
Nov 23, 2016
| can_optimize = !reprs.iter().any(|r| { | ||
| match *r { | ||
| attr::ReprAny => false, | ||
| attr::ReprInt(_) => false, |
This comment has been minimized.
This comment has been minimized.
eddyb
Nov 23, 2016
Member
Should use | between these patterns to avoid repeating false (and true below).
This comment has been minimized.
This comment has been minimized.
|
Fixing your code for this change is a fairly trivial matter so I'm of the opinion that the current timetable is fine. Just make the PSA prominent enough (reddit, rust blog, urlo, and irlo) and everything should be fine. |
This comment has been minimized.
This comment has been minimized.
|
I do not think this should be in the next beta, should the next beta be soon. I do not have the schedules memorized yet. This code breaks other people's broken unsafe code in fun ways, but it may also break the compiler. I am not confident enough that this code doesn't create compiler bugs to wish to shove it into a beta as quickly as possible. If @eddyb or @nikomatsakis thinks we're fine, then go for it. But I'd hold off, just in case. As for breaking unsafe code, there were at least 2 places in the compiler with missing |
This comment has been minimized.
This comment has been minimized.
|
To clarify, the 1.15 beta branch has not been forked off yet. That was scheduled to happen some time during the next week. @camlorn is there perhaps a "boolean flag" style switch somewhere we could turn this optimization off on beta easily? If so we could leave this on master, easily backport a "turn off this optimization" to beta, and let this ride the trains to 1.16 stable. |
This comment has been minimized.
This comment has been minimized.
|
@alexcrichton There's a check for |
This comment has been minimized.
This comment has been minimized.
|
Ok, so we have a few options now. I'm assuming that this PR itself is going into the 1.15 beta no matter what.
|
This comment has been minimized.
This comment has been minimized.
|
I can open another PR which just turns it off. All you have to do is short-circuit one condition in |
camlorn
deleted the
camlorn:univariant_layout_optimization
branch
Dec 18, 2016
This comment has been minimized.
This comment has been minimized.
|
I was wrong. Turning this off is a little complicated. If we do, some tests are going to fail, so we'll have to restore the old versions. |
This comment has been minimized.
This comment has been minimized.
|
@camlorn can't we add a compiler flag ( In retrospect I wish I had raised this to the core team to discuss proper handling. |
This comment has been minimized.
This comment has been minimized.
|
@camlorn it looks like this may have regressed a test on Windows AppVeyor, specifically:
That wouldn't happen to look familiar off-hand, would it? |
This comment has been minimized.
This comment has been minimized.
|
@nikomatsakis I'm going to see if I can figure out how to reproduce this locally. |
This comment has been minimized.
This comment has been minimized.
|
Ok turns out we're accidentally not running debuginfo tests on the bots. That may mean that this is trivially reproducible, though. |
alexcrichton
referenced this pull request
Dec 19, 2016
Merged
rustbuild: Run debuginfo tests by default #38471
This comment has been minimized.
This comment has been minimized.
|
Upon following the readme's directions verbatim, I get this. Consequently, I can't reproduce. If I don't use rustbuild, it also happens, but later in the process. I checked against WSL: the broken test passes there. Taking a cursory look at the test, I don't see anything that could be different based on platform. These are also both using gdb. |
jdm
referenced this pull request
Dec 20, 2016
Merged
Update Rust to 1.15.0-nightly (71c06a56a 2016-12-18) #14633
This comment has been minimized.
This comment has been minimized.
|
@camlorn the |
This comment has been minimized.
This comment has been minimized.
|
@gnzlbg |
This comment has been minimized.
This comment has been minimized.
|
@camlorn go for it, you already did the work, those are just the 3-5 easy lines of code to make sure nobody else screws it up, totally worth it |
This comment has been minimized.
This comment has been minimized.
|
@gnzlbg |
camlorn commentedOct 27, 2016
This is work in progress. The goal is to divorce the order of fields in source code from the order of fields in the LLVM IR, then optimize structs (and tuples/enum variants)by always ordering fields from least to most aligned. It does not work yet. I intend to check compiler memory usage as a benchmark, and a crater run will probably be required.
I don't know enough of the compiler to complete this work unaided. If you see places that still need updating, please mention them. The only one I know of currently is debuginfo, which I'm putting off intentionally until a bit later.
r? @eddyb