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

Refactor layout to store offsets of fields, not offsets after fields #36904

Merged
merged 1 commit into from Oct 3, 2016

Conversation

ahicks92
Copy link
Contributor

@ahicks92 ahicks92 commented Oct 2, 2016

This is the next PR moving us towards being able to reorder struct fields.

The old code implicitly stored the offset of the first field. This is inadequate because the first field may no longer be offset 0 in future. This PR refactors layout to use a offsets vector instead of a offset_after_field vector.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

// FIXME(eddyb) use small vector optimization for the common case.
pub offset_after_field: Vec<Size>
/// Offsets for the first byte of each field.
pub offsets: Vec<Size>,
Copy link
Member

Choose a reason for hiding this comment

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

Does eddyb's fixme still apply?
// FIXME(eddyb) use small vector optimization for the common case.

Copy link
Member

Choose a reason for hiding this comment

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

Yes it does.

@ahicks92
Copy link
Contributor Author

ahicks92 commented Oct 2, 2016

@mark-buer
Probably, but I'm not concerned with that here unless those functions end up being broken.

The small vector optimization isn't about that struct, it's about avoiding a heap allocation in the compiler itself in some cases. I don't think it's worth it, unless someone adds something to the compiler that can be used in all cases where small vector optimization might be beneficial.

@ahicks92
Copy link
Contributor Author

ahicks92 commented Oct 2, 2016

Also, I should clarify: this isn't quite finalized yet.

Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

This looks great! I only had nits & minor extra cleanups to mention.

// FIXME(eddyb) use small vector optimization for the common case.
pub offset_after_field: Vec<Size>
/// Offsets for the first byte of each field.
pub offsets: Vec<Size>,
Copy link
Member

Choose a reason for hiding this comment

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

Yes it does.

/// Offsets for the first byte of each field.
pub offsets: Vec<Size>,

min_size: Size,
Copy link
Member

Choose a reason for hiding this comment

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

No reason to keep this private and require the accessor.

self.offset_after_field[index-1]
}
assert!(index < self.offsets.len());
self.offsets[index]
}
Copy link
Member

Choose a reason for hiding this comment

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

Only reason for this method was the oddity of offset_after_field, direct access to offsets would be better.

if fields > 1 {
variant.offset_after_field[fields - 2].bytes()
variant.offsets[fields - 1].bytes()
} else {
0
}
Copy link
Member

Choose a reason for hiding this comment

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

This whole thing can be variant.offsets.last().map_or(0, |o| o.bytes()) - and local_prefix_bytes doesn't even need to exist as a separate function.

@ahicks92
Copy link
Contributor Author

ahicks92 commented Oct 2, 2016

I mixed up the SVO FIXME comments. My bad.

Is the small vector optimization anywhere outside layout? It seems to me that the way to fix this is to write something that implements a subset of Vec's API if we want it.

@ahicks92 ahicks92 force-pushed the field_offsets_refactor branch from 471352c to 9482bce Compare Oct 2, 2016
@ahicks92
Copy link
Contributor Author

ahicks92 commented Oct 2, 2016

The first set of review comments should be incorporated. I squashed the history as well, so it's one commit.

Think all that's maybe left is formatting.

eddyb
eddyb approved these changes Oct 2, 2016
@eddyb
Copy link
Member

eddyb commented Oct 2, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Oct 2, 2016

📌 Commit 9482bce has been approved by eddyb

@bors
Copy link
Contributor

bors commented Oct 2, 2016

Testing commit 9482bce with merge 1cdc0fb...

bors added a commit that referenced this pull request Oct 2, 2016
Refactor layout to store offsets of fields, not offsets after fields

This is the next PR moving us towards being able to reorder struct fields.

The old code implicitly stored the offset of the first field.  This is inadequate because the first field may no longer be offset 0 in future.  This PR refactors `layout` to use a `offsets` vector instead of a `offset_after_field` vector.
@bors bors merged commit 9482bce into rust-lang:master Oct 3, 2016
@ahicks92 ahicks92 deleted the field_offsets_refactor branch Oct 3, 2016
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.

None yet

6 participants