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

Add debug flag `-Z print-type-sizes` for instrumention type/variant sizes #37770

Merged
merged 3 commits into from Nov 24, 2016

Conversation

Projects
None yet
9 participants
@pnkfelix
Member

pnkfelix commented Nov 14, 2016

Add debug flag -Z print-type-sizes for instrumention type/variant sizes

This is meant to help with things like #36799 in a very local way; namely, once you have a hypothesis as to which types have a large population or are "too large", you can use -Z print-type-sizes to learn how large each type is, and how much each variant in an enum contributes to the size of that overall enum.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Nov 14, 2016

Collaborator

r? @arielb1

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

Collaborator

rust-highfive commented Nov 14, 2016

r? @arielb1

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

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Nov 14, 2016

Member

For example, applying this tool to the rustc crate tells us things like:

...
print-type-size t: `mir::StatementKind<'tcx>` overall bytes: 176
print-type-size    variant[0] exact bytes: 176
print-type-size    variant[1] exact bytes: 32
print-type-size    variant[2] exact bytes: 24
print-type-size    variant[3] exact bytes: 24
print-type-size    variant[4] exact bytes: 8
...
print-type-size t: `mir::Rvalue<'tcx>` overall bytes: 152
print-type-size    variant[0] exact bytes: 80
print-type-size    variant[1] exact bytes: 120
print-type-size    variant[2] exact bytes: 40
print-type-size    variant[3] exact bytes: 24
print-type-size    variant[4] exact bytes: 88
print-type-size    variant[5] exact bytes: 152
print-type-size    variant[6] exact bytes: 152
print-type-size    variant[7] exact bytes: 80
print-type-size    variant[8] exact bytes: 16
print-type-size    variant[9] exact bytes: 88
print-type-size    variant[10] exact bytes: 144
...
print-type-size t: `mir::Lvalue<'tcx>` overall bytes: 16
...

So, we could reduce the space impact of non-Assign MIR statements by boxing the Rvalue in the StatementKind::Assign variant.

Likewise, we could reduce the space impact of Rvalue itself by refactoring InlineAsm, CheckedBinaryOp, BinaryOp, and Repeat ...

Member

pnkfelix commented Nov 14, 2016

For example, applying this tool to the rustc crate tells us things like:

...
print-type-size t: `mir::StatementKind<'tcx>` overall bytes: 176
print-type-size    variant[0] exact bytes: 176
print-type-size    variant[1] exact bytes: 32
print-type-size    variant[2] exact bytes: 24
print-type-size    variant[3] exact bytes: 24
print-type-size    variant[4] exact bytes: 8
...
print-type-size t: `mir::Rvalue<'tcx>` overall bytes: 152
print-type-size    variant[0] exact bytes: 80
print-type-size    variant[1] exact bytes: 120
print-type-size    variant[2] exact bytes: 40
print-type-size    variant[3] exact bytes: 24
print-type-size    variant[4] exact bytes: 88
print-type-size    variant[5] exact bytes: 152
print-type-size    variant[6] exact bytes: 152
print-type-size    variant[7] exact bytes: 80
print-type-size    variant[8] exact bytes: 16
print-type-size    variant[9] exact bytes: 88
print-type-size    variant[10] exact bytes: 144
...
print-type-size t: `mir::Lvalue<'tcx>` overall bytes: 16
...

So, we could reduce the space impact of non-Assign MIR statements by boxing the Rvalue in the StatementKind::Assign variant.

Likewise, we could reduce the space impact of Rvalue itself by refactoring InlineAsm, CheckedBinaryOp, BinaryOp, and Repeat ...

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Nov 14, 2016

Member

InlineAsm showing up again is interesting, since it also was the largest variant of Expr in both HIR and the libsyntax AST - I believe I screwed up and ended up embedding a huge struct directly there.

So, we could reduce the space impact of non-Assign MIR statements by boxing the Rvalue in the StatementKind::Assign variant.

FWIW if someone is looking at this - please don't take this as a suggestion, most statements are Assign, so this change would likely increase memory usage of MIR, potentially doubling it in some cases.

Member

eddyb commented Nov 14, 2016

InlineAsm showing up again is interesting, since it also was the largest variant of Expr in both HIR and the libsyntax AST - I believe I screwed up and ended up embedding a huge struct directly there.

So, we could reduce the space impact of non-Assign MIR statements by boxing the Rvalue in the StatementKind::Assign variant.

FWIW if someone is looking at this - please don't take this as a suggestion, most statements are Assign, so this change would likely increase memory usage of MIR, potentially doubling it in some cases.

@Mark-Simulacrum

This comment has been minimized.

Show comment
Hide comment
@Mark-Simulacrum

Mark-Simulacrum Nov 14, 2016

Member

Is there a way we could print the actual variant names instead of variant[0]?

Member

Mark-Simulacrum commented Nov 14, 2016

Is there a way we could print the actual variant names instead of variant[0]?

@arielb1

Nice approach.

Show outdated Hide outdated src/librustc_mir/transform/print_type_sizes.rs
return;
}
let mut visitor = TypeVisitor {

This comment has been minimized.

@arielb1

arielb1 Nov 14, 2016

Contributor

Shouldn't we have a single seen list for the entire program? (A: this is just a cache for the deduplication in session?).

@arielb1

arielb1 Nov 14, 2016

Contributor

Shouldn't we have a single seen list for the entire program? (A: this is just a cache for the deduplication in session?).

Show outdated Hide outdated src/librustc_mir/transform/print_type_sizes.rs
tcx: TyCtxt<'a, 'tcx, 'tcx>,
mir: &Mir<'tcx>) {
if tcx.sess.err_count() > 0 {
// compiling a broken program can obviously result in a

This comment has been minimized.

@arielb1

arielb1 Nov 14, 2016

Contributor

nit: comment copy-pasta

@arielb1

arielb1 Nov 14, 2016

Contributor

nit: comment copy-pasta

Show outdated Hide outdated src/librustc/session/mod.rs
overall_size: overall_size,
variant_sizes: sizes,
};
if !self.type_sizes.contains(&info) {

This comment has been minimized.

@arielb1

arielb1 Nov 14, 2016

Contributor

risk of O(n^2). Because we sort this anyway, why not use a HashSet?

@arielb1

arielb1 Nov 14, 2016

Contributor

risk of O(n^2). Because we sort this anyway, why not use a HashSet?

This comment has been minimized.

@pnkfelix

pnkfelix Nov 15, 2016

Member

Yeah true; at one time for some reason I thought there was value in preserving the order in which we encountered the types, but that's obviously not reflected in the current output.

@pnkfelix

pnkfelix Nov 15, 2016

Member

Yeah true; at one time for some reason I thought there was value in preserving the order in which we encountered the types, but that's obviously not reflected in the current output.

Show outdated Hide outdated src/librustc/session/mod.rs
}
}
pub fn sort_by_type_description(&mut self) {

This comment has been minimized.

@arielb1

arielb1 Nov 14, 2016

Contributor

nit: I would prefer this entire pipeline to go here.

@arielb1

arielb1 Nov 14, 2016

Contributor

nit: I would prefer this entire pipeline to go here.

This comment has been minimized.

@pnkfelix

pnkfelix Nov 15, 2016

Member

I'll combine it all into fn print_type_sizes as part of the switch to an underlying HashSet.

@pnkfelix

pnkfelix Nov 15, 2016

Member

I'll combine it all into fn print_type_sizes as part of the switch to an underlying HashSet.

@@ -216,6 +216,13 @@ pub fn compile_input(sess: &Session,
})??
};
if sess.opts.debugging_opts.print_type_sizes {

This comment has been minimized.

@arielb1

arielb1 Nov 14, 2016

Contributor

Shouldn't we do this in the "post-trans" closure a few lines above?

@arielb1

arielb1 Nov 14, 2016

Contributor

Shouldn't we do this in the "post-trans" closure a few lines above?

This comment has been minimized.

@pnkfelix

pnkfelix Nov 15, 2016

Member

The closure we pass into phase_3_run_analysis_passes?

What is the benefit you see of putting it in there? Is there some internal instrumentation that needs to be aware of this print pass taking place?

@pnkfelix

pnkfelix Nov 15, 2016

Member

The closure we pass into phase_3_run_analysis_passes?

What is the benefit you see of putting it in there? Is there some internal instrumentation that needs to be aware of this print pass taking place?

This comment has been minimized.

@arielb1

arielb1 Nov 15, 2016

Contributor

It just looks better there. Also, then you could have the type size list on the global tcx.

@arielb1

arielb1 Nov 15, 2016

Contributor

It just looks better there. Also, then you could have the type size list on the global tcx.

Show outdated Hide outdated src/librustc_mir/transform/print_type_sizes.rs
impl Pass for GatherTypeSizesMir {
}
impl<'tcx> MirPassHook<'tcx> for GatherTypeSizesMir {

This comment has been minimized.

@arielb1

arielb1 Nov 14, 2016

Contributor

There is no reason to implement MirPassHook - you are not using it anywhere.

@arielb1

arielb1 Nov 14, 2016

Contributor

There is no reason to implement MirPassHook - you are not using it anywhere.

@@ -0,0 +1,12 @@
// All of the types that occur in this function are uninteresting, in

This comment has been minimized.

@arielb1

arielb1 Nov 14, 2016

Contributor

Nice tests.

@arielb1

arielb1 Nov 14, 2016

Contributor

Nice tests.

This comment has been minimized.

@arielb1

arielb1 Nov 14, 2016

Contributor

Don't you need copyright comments though?

@arielb1

arielb1 Nov 14, 2016

Contributor

Don't you need copyright comments though?

This comment has been minimized.

@pnkfelix

pnkfelix Nov 15, 2016

Member

Oh yes, I totally forgot the copyright comments; will fix.

@pnkfelix

pnkfelix Nov 15, 2016

Member

Oh yes, I totally forgot the copyright comments; will fix.

Show outdated Hide outdated src/librustc_mir/transform/print_type_sizes.rs
self.seen.insert(ty);
let reveal = Reveal::All;
// let reveal = Reveal::NotSpecializable;

This comment has been minimized.

@arielb1

arielb1 Nov 14, 2016

Contributor

nit: commented-out code in program.

@arielb1

arielb1 Nov 14, 2016

Contributor

nit: commented-out code in program.

Show outdated Hide outdated src/librustc_mir/transform/print_type_sizes.rs
Vec::new()
}
// RawNullablePointer/StructWrappedNullablePointer

This comment has been minimized.

@arielb1

arielb1 Nov 14, 2016

Contributor

Exhaustive match?

@arielb1

arielb1 Nov 14, 2016

Contributor

Exhaustive match?

Show outdated Hide outdated src/librustc_mir/transform/print_type_sizes.rs
variant_sizes);
}
Err(err) => {
self.tcx.sess.warn(&format!("print-type-size t: `{:?}` err: {:?}", ty, err));

This comment has been minimized.

@arielb1

arielb1 Nov 14, 2016

Contributor

span_warn? At least use the MIR's span.

@arielb1

arielb1 Nov 14, 2016

Contributor

span_warn? At least use the MIR's span.

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Nov 14, 2016

Member

FWIW I'd prefer this to either work like the existing variant size lint (looking at monomorphic definitions) or by inspecting the type layout cache after trans. Although I don't mind the current implementation, it feels more thorough, in an analysis way rather than a debug way.

Member

eddyb commented Nov 14, 2016

FWIW I'd prefer this to either work like the existing variant size lint (looking at monomorphic definitions) or by inspecting the type layout cache after trans. Although I don't mind the current implementation, it feels more thorough, in an analysis way rather than a debug way.

@nnethercote

This comment has been minimized.

Show comment
Hide comment
@nnethercote

nnethercote Nov 15, 2016

Contributor

Great to see you working on this, @pnkfelix.

My suggestions for this feature are in #37623 (comment). That example has a bit more detail, including variant names and the size of individual fields & the padding between them. Basically, assuming it's possible, don't leave the reader having to infer anything about the layout.

Contributor

nnethercote commented Nov 15, 2016

Great to see you working on this, @pnkfelix.

My suggestions for this feature are in #37623 (comment). That example has a bit more detail, including variant names and the size of individual fields & the padding between them. Basically, assuming it's possible, don't leave the reader having to infer anything about the layout.

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Nov 15, 2016

Member

@nnethercote thanks for the pointer to that issue. I'll see what I can do to try to make this output include more of the information you desire there

Member

pnkfelix commented Nov 15, 2016

@nnethercote thanks for the pointer to that issue. I'll see what I can do to try to make this output include more of the information you desire there

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Nov 15, 2016

Member

r=me if @arielb1's and @nnethercote's concerns have been addressed.

Member

eddyb commented Nov 15, 2016

r=me if @arielb1's and @nnethercote's concerns have been addressed.

@nnethercote

This comment has been minimized.

Show comment
Hide comment
@nnethercote

nnethercote Nov 15, 2016

Contributor

@pnkfelix, can you show some sample output for the updated code? Thank you.

Contributor

nnethercote commented Nov 15, 2016

@pnkfelix, can you show some sample output for the updated code? Thank you.

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Nov 16, 2016

Member

@nnethercote I haven't incorporated your ideas for the output yet. You can see the sample output in the ui tests that are in the PR.

Member

pnkfelix commented Nov 16, 2016

@nnethercote I haven't incorporated your ideas for the output yet. You can see the sample output in the ui tests that are in the PR.

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Nov 16, 2016

Contributor

I'll give this an r+ for now and let's implement field-by-field size output in a separate PR.

@bors r+

Contributor

arielb1 commented Nov 16, 2016

I'll give this an r+ for now and let's implement field-by-field size output in a separate PR.

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 16, 2016

Contributor

📌 Commit 089fbcf has been approved by arielb1

Contributor

bors commented Nov 16, 2016

📌 Commit 089fbcf has been approved by arielb1

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Nov 18, 2016

Member

(odd, I'm not clear why travis is failing in that manner; does it test a merge, and some of my imports have been taken away...?)

Update: yeah that must have been what was going on; sorry for the noise.

Member

pnkfelix commented Nov 18, 2016

(odd, I'm not clear why travis is failing in that manner; does it test a merge, and some of my imports have been taken away...?)

Update: yeah that must have been what was going on; sorry for the noise.

@nnethercote

This is looking great! Thank you for adding the extra detail, it really helps.

I have a few nitpicks about the output below that I think will improve readability somewhat.

Show outdated Hide outdated src/test/ui/print_type_sizes/padding.stdout
@@ -0,0 +1,21 @@
print-type-size type: `E1` overall bytes: 24 align: 8
print-type-size discrimimant bytes: 4

This comment has been minimized.

@nnethercote

nnethercote Nov 18, 2016

Contributor

You misspelled "discriminant".

@nnethercote

nnethercote Nov 18, 2016

Contributor

You misspelled "discriminant".

This comment has been minimized.

@pnkfelix

pnkfelix Nov 18, 2016

Member

ha ha, whoops...

@pnkfelix

pnkfelix Nov 18, 2016

Member

ha ha, whoops...

Show outdated Hide outdated src/test/ui/print_type_sizes/padding.stdout
print-type-size discrimimant bytes: 4
print-type-size variant A exact bytes: 16
print-type-size padding bytes: 4
print-type-size field .0 bytes: 8 align: 8

This comment has been minimized.

@nnethercote

nnethercote Nov 18, 2016

Contributor

AIUI rustc is going to be able to rearrange field order in the future to improve packing. In that case would the field numbers be listed out of order?

@nnethercote

nnethercote Nov 18, 2016

Contributor

AIUI rustc is going to be able to rearrange field order in the future to improve packing. In that case would the field numbers be listed out of order?

This comment has been minimized.

@pnkfelix

pnkfelix Nov 18, 2016

Member

This code in principle can print the offset assigned to each field (the API includes the offsets being passed back). Or we can sort the variants by offset, and derive the implicit padding accordingly.

However my own experiments indicate that the computed offsets from the layout module do not always match what we use in the representation, so I decided not to print out the offsets in this version of the code.

Whenever we get around to rearranging the field order, we will have to fix the layout module to actually compute/maintain correct offset values, and we can revise this accordingly then.

@pnkfelix

pnkfelix Nov 18, 2016

Member

This code in principle can print the offset assigned to each field (the API includes the offsets being passed back). Or we can sort the variants by offset, and derive the implicit padding accordingly.

However my own experiments indicate that the computed offsets from the layout module do not always match what we use in the representation, so I decided not to print out the offsets in this version of the code.

Whenever we get around to rearranging the field order, we will have to fix the layout module to actually compute/maintain correct offset values, and we can revise this accordingly then.

Show outdated Hide outdated src/test/ui/print_type_sizes/nullable.stdout
print-type-size field .post bytes: 8
print-type-size type: `MyOption<IndirectNonZero<u32>>` overall bytes: 40 align: 8
print-type-size variant Some exact bytes: 40
print-type-size field .0 bytes: 40

This comment has been minimized.

@nnethercote

nnethercote Nov 18, 2016

Contributor

Some nitpicks:

  • Do the "overall" and "exact" words contribute anything here? Can they be removed without loss of information?
  • Inconsistent colon placement: in a different place for "type" compared to "field" and "variant".
  • Perhaps the "bytes" should appear after the numbers, on the right-hand side.
  • Field names should have backticks around them, like type names.
  • A comma before the alignment would help readability.
  • "alignment" is probably better than "align".
  • First indent is 3 spaces, second indent is 4 spaces. Use 4 (or 2) consistently.

E.g.:

print-type-size type `MyOption<IndirectNonZero<u32>>`: 40 bytes, alignment: 8 bytes
print-type-size     variant `Some`: 40 bytes
print-type-size         field .0: 40 bytes
@nnethercote

nnethercote Nov 18, 2016

Contributor

Some nitpicks:

  • Do the "overall" and "exact" words contribute anything here? Can they be removed without loss of information?
  • Inconsistent colon placement: in a different place for "type" compared to "field" and "variant".
  • Perhaps the "bytes" should appear after the numbers, on the right-hand side.
  • Field names should have backticks around them, like type names.
  • A comma before the alignment would help readability.
  • "alignment" is probably better than "align".
  • First indent is 3 spaces, second indent is 4 spaces. Use 4 (or 2) consistently.

E.g.:

print-type-size type `MyOption<IndirectNonZero<u32>>`: 40 bytes, alignment: 8 bytes
print-type-size     variant `Some`: 40 bytes
print-type-size         field .0: 40 bytes

This comment has been minimized.

@pnkfelix

pnkfelix Nov 18, 2016

Member
  • The "exact" is there because the layout code makes a distinction between minimum size for a type (which is used for dynamically sized types) vs exact size. However, I never actually made a test that illustrates this, and furthermore, it does not seem to ever arise in my runs on the rustc crate, so maybe that is all dead code for the purposes of this analysis.
  • You're right about the colon placement. I'll think about your suggestion. (My instinct was to instead add colons after "variant" and "field" but your sample looks okay to me...)
  • I can move the "bytes" around to occur after the number, sure.
  • My reasoning behind the backticks is that we only need them for the types (since those have substructure in the form of their instantiated type parameters); the field and variant names never have any substructure, they are always just identifiers or numbers (I think?) so I figured no backticks needed. Note that in your sample you did not put ".0"
  • A comma before align/alignment sounds fine.
  • Thanks, I thought I fixed the inconsistent indents but clearly never bothered to count spaces in the actual output in the end.
@pnkfelix

pnkfelix Nov 18, 2016

Member
  • The "exact" is there because the layout code makes a distinction between minimum size for a type (which is used for dynamically sized types) vs exact size. However, I never actually made a test that illustrates this, and furthermore, it does not seem to ever arise in my runs on the rustc crate, so maybe that is all dead code for the purposes of this analysis.
  • You're right about the colon placement. I'll think about your suggestion. (My instinct was to instead add colons after "variant" and "field" but your sample looks okay to me...)
  • I can move the "bytes" around to occur after the number, sure.
  • My reasoning behind the backticks is that we only need them for the types (since those have substructure in the form of their instantiated type parameters); the field and variant names never have any substructure, they are always just identifiers or numbers (I think?) so I figured no backticks needed. Note that in your sample you did not put ".0"
  • A comma before align/alignment sounds fine.
  • Thanks, I thought I fixed the inconsistent indents but clearly never bothered to count spaces in the actual output in the end.

This comment has been minimized.

@pnkfelix

pnkfelix Nov 18, 2016

Member

(I'll go ahead and try putting in all of the suggestions and see how the output fares. I think my biggest issue might be with adding backticks to the variants and fields, but I'm willing to give it a shot.)

@pnkfelix

pnkfelix Nov 18, 2016

Member

(I'll go ahead and try putting in all of the suggestions and see how the output fares. I think my biggest issue might be with adding backticks to the variants and fields, but I'm willing to give it a shot.)

This comment has been minimized.

@nnethercote

nnethercote Nov 18, 2016

Contributor

Sounds fine. I thought about suggesting backticks for the field names too, but the '.' seemed like a sufficient marker. But backticks around types and fields and variants is another possibility. I'll let you decide; there's not going to be One True Way to print this data :)

@nnethercote

nnethercote Nov 18, 2016

Contributor

Sounds fine. I thought about suggesting backticks for the field names too, but the '.' seemed like a sufficient marker. But backticks around types and fields and variants is another possibility. I'll let you decide; there's not going to be One True Way to print this data :)

Show outdated Hide outdated src/librustc/session/code_stats.rs
if offset != _lying_offset {
// Experiments indicate that the offset extracted from Layout does
// not always correctly account for padding. Thus the name :(
debug!("offset: {} _lying_offset: {}", offset, _lying_offset);

This comment has been minimized.

@arielb1

arielb1 Nov 19, 2016

Contributor

Sounds like trouble. When does this happen?

Could you reproduce this?

@arielb1

arielb1 Nov 19, 2016

Contributor

Sounds like trouble. When does this happen?

Could you reproduce this?

This comment has been minimized.

@pnkfelix

pnkfelix Nov 19, 2016

Member

(Note: This comment has been revised several times; the most important change was correcting the listed path to the test to actually include the filename.)

It reproduces regularly on the test/ui/print_type_sizes/padding.rs test. Output is like this when you turn the debug logging on:

print-type-size type: `E1`: 24 bytes, alignment: 8 bytes
print-type-size     discriminant: 4 bytes
print-type-size     variant `A`: 16 bytes
DEBUG:rustc::session::code_stats: offset: 8 _lying_offset: 0
print-type-size         padding bytes: 4
print-type-size         field `.0`: 8 bytes, alignment: 8 bytes
DEBUG:rustc::session::code_stats: offset: 16 _lying_offset: 8
print-type-size         field `.1`: 4 bytes
print-type-size     variant `B`: 8 bytes
DEBUG:rustc::session::code_stats: offset: 4 _lying_offset: 0
print-type-size         field `.0`: 8 bytes
print-type-size     end padding bytes: 4
print-type-size type: `E2`: 16 bytes, alignment: 8 bytes
print-type-size     discriminant: 4 bytes
print-type-size     variant `A`: 12 bytes
DEBUG:rustc::session::code_stats: offset: 4 _lying_offset: 0
print-type-size         field `.0`: 4 bytes
DEBUG:rustc::session::code_stats: offset: 8 _lying_offset: 4
print-type-size         field `.1`: 8 bytes
print-type-size     variant `B`: 8 bytes
DEBUG:rustc::session::code_stats: offset: 4 _lying_offset: 0
print-type-size         field `.0`: 8 bytes
print-type-size type: `S`: 8 bytes, alignment: 4 bytes
print-type-size     field `.a`: 1 bytes
print-type-size     field `.b`: 1 bytes
print-type-size     padding bytes: 2
print-type-size     field `.g`: 4 bytes, alignment: 4 bytes

(My assertion that the reported offset is "lying" is based on manual inspection of a value that I transmuted to a tuple.)

I've been assuming that this is not currently as serious a bug as one might think because I've been assuming that nothing important is using the offset values computed by the layout module. However, as I mentioned to @nnethercote in an (gh-marked "outdated") comment above,

Whenever we get around to rearranging the field order, we will have to fix the layout module to actually compute/maintain correct offset values, and we can revise this accordingly then.

@pnkfelix

pnkfelix Nov 19, 2016

Member

(Note: This comment has been revised several times; the most important change was correcting the listed path to the test to actually include the filename.)

It reproduces regularly on the test/ui/print_type_sizes/padding.rs test. Output is like this when you turn the debug logging on:

print-type-size type: `E1`: 24 bytes, alignment: 8 bytes
print-type-size     discriminant: 4 bytes
print-type-size     variant `A`: 16 bytes
DEBUG:rustc::session::code_stats: offset: 8 _lying_offset: 0
print-type-size         padding bytes: 4
print-type-size         field `.0`: 8 bytes, alignment: 8 bytes
DEBUG:rustc::session::code_stats: offset: 16 _lying_offset: 8
print-type-size         field `.1`: 4 bytes
print-type-size     variant `B`: 8 bytes
DEBUG:rustc::session::code_stats: offset: 4 _lying_offset: 0
print-type-size         field `.0`: 8 bytes
print-type-size     end padding bytes: 4
print-type-size type: `E2`: 16 bytes, alignment: 8 bytes
print-type-size     discriminant: 4 bytes
print-type-size     variant `A`: 12 bytes
DEBUG:rustc::session::code_stats: offset: 4 _lying_offset: 0
print-type-size         field `.0`: 4 bytes
DEBUG:rustc::session::code_stats: offset: 8 _lying_offset: 4
print-type-size         field `.1`: 8 bytes
print-type-size     variant `B`: 8 bytes
DEBUG:rustc::session::code_stats: offset: 4 _lying_offset: 0
print-type-size         field `.0`: 8 bytes
print-type-size type: `S`: 8 bytes, alignment: 4 bytes
print-type-size     field `.a`: 1 bytes
print-type-size     field `.b`: 1 bytes
print-type-size     padding bytes: 2
print-type-size     field `.g`: 4 bytes, alignment: 4 bytes

(My assertion that the reported offset is "lying" is based on manual inspection of a value that I transmuted to a tuple.)

I've been assuming that this is not currently as serious a bug as one might think because I've been assuming that nothing important is using the offset values computed by the layout module. However, as I mentioned to @nnethercote in an (gh-marked "outdated") comment above,

Whenever we get around to rearranging the field order, we will have to fix the layout module to actually compute/maintain correct offset values, and we can revise this accordingly then.

This comment has been minimized.

@pnkfelix

pnkfelix Nov 19, 2016

Member

Note in particular the main inconsistency illustrated here is that the so-called "lying-offset" does not account for (1) the discriminant nor (2) the padding injected (due to the discriminant) before the first field of a variant.

When I first looked at this I thought that the problem was just the first noted issue, which is trivially dealt with by adding the discriminant size to the offset. But the second issue is the killer: the reported offsets really should include any injected padding as well, and do not in this case.

@pnkfelix

pnkfelix Nov 19, 2016

Member

Note in particular the main inconsistency illustrated here is that the so-called "lying-offset" does not account for (1) the discriminant nor (2) the padding injected (due to the discriminant) before the first field of a variant.

When I first looked at this I thought that the problem was just the first noted issue, which is trivially dealt with by adding the discriminant size to the offset. But the second issue is the killer: the reported offsets really should include any injected padding as well, and do not in this case.

This comment has been minimized.

@pnkfelix

pnkfelix Nov 21, 2016

Member

The answer ended up being that I was misunderstanding the representation being used in the layout module.

@pnkfelix

pnkfelix Nov 21, 2016

Member

The answer ended up being that I was misunderstanding the representation being used in the layout module.

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix
Member

pnkfelix commented Nov 21, 2016

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 21, 2016

Contributor

📌 Commit 559c135 has been approved by pnkfelix

Contributor

bors commented Nov 21, 2016

📌 Commit 559c135 has been approved by pnkfelix

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Nov 21, 2016

Member

@bors r=ariebl1

Member

pnkfelix commented Nov 21, 2016

@bors r=ariebl1

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 21, 2016

Contributor

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #37824
Contributor

bors commented Nov 21, 2016

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #37824
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 21, 2016

Contributor

📌 Commit 559c135 has been approved by ariebl1

Contributor

bors commented Nov 21, 2016

📌 Commit 559c135 has been approved by ariebl1

@TimNN

This comment has been minimized.

Show comment
Hide comment
@TimNN

TimNN Nov 21, 2016

Contributor

I think this is missing anonymous.stdout for the UI tests?

Edit: Nevermind, that test isn't supposed to produce any output, is it? (In which case UI test will treat a missing file == empty output?).

Edit2: Shouldn't it still have // compile-flags: -Z print-type-sizes?

Contributor

TimNN commented Nov 21, 2016

I think this is missing anonymous.stdout for the UI tests?

Edit: Nevermind, that test isn't supposed to produce any output, is it? (In which case UI test will treat a missing file == empty output?).

Edit2: Shouldn't it still have // compile-flags: -Z print-type-sizes?

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Nov 21, 2016

Contributor

@bors r+

Contributor

arielb1 commented Nov 21, 2016

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 21, 2016

Contributor

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #37912
Contributor

bors commented Nov 21, 2016

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #37912
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 21, 2016

Contributor

📌 Commit 559c135 has been approved by arielb1

Contributor

bors commented Nov 21, 2016

📌 Commit 559c135 has been approved by arielb1

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 22, 2016

Contributor

⌛️ Testing commit 559c135 with merge 7189dbb...

Contributor

bors commented Nov 22, 2016

⌛️ Testing commit 559c135 with merge 7189dbb...

bors added a commit that referenced this pull request Nov 22, 2016

Auto merge of #37770 - pnkfelix:print-type-sizes, r=arielb1
Add debug flag `-Z print-type-sizes` for instrumention type/variant sizes

Add debug flag `-Z print-type-sizes` for instrumention type/variant sizes

This is meant to help with things like #36799 in a very local way; namely, once you have a hypothesis as to which types have a large population or are "too large", you can use `-Z print-type-sizes` to learn how large each type is, and how much each variant in an enum contributes to the size of that overall enum.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 22, 2016

Contributor

💔 Test failed - auto-mac-32-opt

Contributor

bors commented Nov 22, 2016

💔 Test failed - auto-mac-32-opt

@sanxiyn

This comment has been minimized.

Show comment
Hide comment
@sanxiyn

sanxiyn Nov 22, 2016

Member

Legitimate failure. Type sizes are (of course) different in 32-bit platforms, and do not match the expected output.

Member

sanxiyn commented Nov 22, 2016

Legitimate failure. Type sizes are (of course) different in 32-bit platforms, and do not match the expected output.

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Nov 23, 2016

Member

@sanxiyn hmm, yes of course. Odd that I didn't see this on my own local (32-bit arm) cross compiles; are u64's 8-byte aligned on arm-32bit?

Member

pnkfelix commented Nov 23, 2016

@sanxiyn hmm, yes of course. Odd that I didn't see this on my own local (32-bit arm) cross compiles; are u64's 8-byte aligned on arm-32bit?

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Nov 23, 2016

Member

@TimNN you are right, anonmous.rs should have // compile-flags: -Z print-type-sizes; its silly right now.

(In the meantime I need to figure out if there's a way for me to encode target specific test output in the ui tests...)

Member

pnkfelix commented Nov 23, 2016

@TimNN you are right, anonmous.rs should have // compile-flags: -Z print-type-sizes; its silly right now.

(In the meantime I need to figure out if there's a way for me to encode target specific test output in the ui tests...)

pnkfelix added some commits Nov 14, 2016

Revisions from review comments, squashed.
Biggest change: Revised print-type-sizes output to include breakdown
of layout.

Includes info about field sizes (and alignment + padding when padding
is injected; the injected padding is derived from the offsets computed
by layout module).

Output format is illustrated in commit that has the ui tests.

Note: there exists (at least) one case of variant w/o name: empty
enums.  Namely, empty enums use anonymous univariant repr. So for such
cases, print the number of the variant instead of the name.

----

Also, eddyb suggested of reading from `layout_cache` post-trans.

(For casual readers: the compiler source often uses the word "cache"
for tables that are in fact not periodically purged, and thus are
useful as the basis for data like this.)

Some types that were previously not printed are now included in the
output. (See e.g. the tests `print_type_sizes/generics.rs` and
`print_type_sizes/variants.rs`)

----

Other review feedback:

switch to an exhaustive match when filtering in just structural types.
switch to hashset for layout info and move sort into print method.

----

Driveby change: Factored session::code_stats into its own module

----

incorporate njn feedback re output formatting.
Tests of `-Z print-type-sizes` functionality.
Note that the tests have been updated to initialize the local
variables; originally it was enough just to declare them.

Back when I started this, the `layout_cache` contained entries even
just for types that had been declared but not initialized. Apparently
things have changed in the interim so that if I want one of those
layouts to be computed, I need to actually initialize the value.

(Incidentally, this shows a weakness in the strategy of just walking
the `layout_cache`; the original strategy of using a MIR visitor would
probably have exhibited more robustness in terms of consistent output,
but it had other weaknesses so I chose not to reimplement it. At
least, not yet.)

----

Also, I have updated tests to avoid target-specific alignments.
@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Nov 24, 2016

Member

@bors r=arielb1

Member

pnkfelix commented Nov 24, 2016

@bors r=arielb1

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 24, 2016

Contributor

📌 Commit 75825fe has been approved by arielb1

Contributor

bors commented Nov 24, 2016

📌 Commit 75825fe has been approved by arielb1

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 24, 2016

Contributor

⌛️ Testing commit 75825fe with merge ddf011d...

Contributor

bors commented Nov 24, 2016

⌛️ Testing commit 75825fe with merge ddf011d...

bors added a commit that referenced this pull request Nov 24, 2016

Auto merge of #37770 - pnkfelix:print-type-sizes, r=arielb1
Add debug flag `-Z print-type-sizes` for instrumention type/variant sizes

Add debug flag `-Z print-type-sizes` for instrumention type/variant sizes

This is meant to help with things like #36799 in a very local way; namely, once you have a hypothesis as to which types have a large population or are "too large", you can use `-Z print-type-sizes` to learn how large each type is, and how much each variant in an enum contributes to the size of that overall enum.

@bors bors merged commit 75825fe into rust-lang:master Nov 24, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment