Skip to content

Codegen implementation of SizeBytes for everything#4758

Merged
teh-cmc merged 12 commits intomainfrom
cmc/codegen_sizebytes
Jan 10, 2024
Merged

Codegen implementation of SizeBytes for everything#4758
teh-cmc merged 12 commits intomainfrom
cmc/codegen_sizebytes

Conversation

@teh-cmc
Copy link
Copy Markdown
Contributor

@teh-cmc teh-cmc commented Jan 9, 2024

  • Loggable: SizeBytes
  • Generate SizeBytes implementation for absolutely everything
  • Improved blanket SizeBytes implementations

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@teh-cmc teh-cmc added 🧑‍💻 dev experience developer experience (excluding CI) ⛃ re_datastore affects the datastore itself 📉 performance Optimization, memory use, etc labels Jan 9, 2024
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jan 9, 2024

Size changes

Name main 4758/merge Change
dna.rrd 645.12 kiB 552.96 kiB -14.29%

@teh-cmc teh-cmc force-pushed the cmc/codegen_sizebytes branch from e6c3f75 to 74864ae Compare January 10, 2024 08:08
@teh-cmc teh-cmc marked this pull request as ready for review January 10, 2024 08:21
Comment on lines +6 to +11
// TODO(cmc): Implementing SizeBytes for this type would require a lot of effort,
// which would be wasted since this is supposed to go away very soon.
#[allow(clippy::manual_assert)] // readability
if cfg!(debug_assertions) {
panic!("EntityPropertiesComponent does not report its size properly");
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, @jleibs ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not Jeremy, but confirmed nonetheless :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

cc @jleibs

Comment on lines +12 to +17
// TODO(cmc): Implementing SizeBytes for this type would require a lot of effort,
// which would be wasted since this is supposed to go away very soon.
#[allow(clippy::manual_assert)] // readability
if cfg!(debug_assertions) {
panic!("ViewportLayout does not report its size properly");
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, @jleibs?

@teh-cmc teh-cmc added the exclude from changelog PRs with this won't show up in CHANGELOG.md label Jan 10, 2024
@Wumpf Wumpf self-requested a review January 10, 2024 08:46
Copy link
Copy Markdown
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

Looking great!

What I didn't understand is how the generated code knows when it has to yield to a manual implementation. E.g. ViewportLayout has no generated SizeBytes but only a manually implemented one in _ext - what magic removes the generated version?

For a while I considered asking heap_size_bytes to default zero because that's common in manual implementations. But I figure the explicitness is better as is

quote!(self.0.heap_size_bytes())
} else {
let quoted_heap_size_bytes = obj.fields.iter().map(|obj_field| {
let field_name = format_ident!("{}", obj_field.name);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the later incarnation did crate::to_pascal_case(. This probably should as well

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The other does that because it's enumerating enum variants! This one is generating struct field names, so not pascal case.

@teh-cmc
Copy link
Copy Markdown
Contributor Author

teh-cmc commented Jan 10, 2024

What I didn't understand is how the generated code knows when it has to yield to a manual implementation. E.g. ViewportLayout has no generated SizeBytes but only a manually implemented one in _ext - what magic removes the generated version?

ViewportLayout uses the serde hack-tribute, that's why it gets ignored.

@teh-cmc teh-cmc merged commit f0f67ab into main Jan 10, 2024
@teh-cmc teh-cmc deleted the cmc/codegen_sizebytes branch January 10, 2024 09:26
teh-cmc added a commit that referenced this pull request Jan 10, 2024
- `Loggable: SizeBytes`
- Generate `SizeBytes` implementation for absolutely everything
- Improved blanket `SizeBytes` implementations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🧑‍💻 dev experience developer experience (excluding CI) exclude from changelog PRs with this won't show up in CHANGELOG.md 📉 performance Optimization, memory use, etc ⛃ re_datastore affects the datastore itself

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Codegen SizeBytes implementation for all components and datatypes

2 participants