Shrink many of our error types #10845
Conversation
Large errors leads to large `Result`s, which leads to a lot of memcpy:s and slow performance.
emilk
added
📉 performance
|
Web viewer built successfully. If applicable, you should also test it:
Note: This comment is updated whenever you push a commit. |
| #[test] | ||
| fn test_error_size() { | ||
| assert!( | ||
| std::mem::size_of::<ChunkError>() <= 72, | ||
| "Size of error is {} bytes. Let's try to keep errors small.", | ||
| std::mem::size_of::<ChunkError>() | ||
| ); | ||
| } |
There was a problem hiding this comment.
All of these can be const assertions instead, which will fail these earlier, at compile time.
| #[test] | |
| fn test_error_size() { | |
| assert!( | |
| std::mem::size_of::<ChunkError>() <= 72, | |
| "Size of error is {} bytes. Let's try to keep errors small.", | |
| std::mem::size_of::<ChunkError>() | |
| ); | |
| } | |
| const _: () = assert!( | |
| std::mem::size_of::<ChunkError>() <= 72, | |
| "Error type is too large. Try to reduce its size by boxing some of its variants.", | |
| ); |
Playground link: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=9c0bead836c6b305b767f4e6f9ed4b6b
There was a problem hiding this comment.
My test prints the size of the struct, which is very useful. This can't be done statically afaict.
Also: they don't need to fail on compile-time. This is a performance problem, but you should be able to test and run your new code without having to care about that.
There was a problem hiding this comment.
My test prints the size of the struct, which is very useful. This can't be done statically afaict.
No, this is possible.
But bringing in a dependency for that seems overkill. Why do you need it to print the size of the type? R-A will report the failure at the assertion site, which is in the same file as the type. You can hover over it in your IDE when the assertion fails at compile time.
Also: they don't need to fail on compile-time. This is a performance problem, but you should be able to test and run your new code without having to care about that.
It's a performance problem, but one that you are now forcing everyone to be aware of and always solve before merging a PR. The sooner they find out about it, the better.
Scenario: You add an error variant, the size now exceeds some arbitrary threshold. You don't notice this. Then you write some code using that error variant, constructing it in different places, mapping to it from different errors, etc. Then you run your tests-- only NOW you find out about the issue. So you have to go back and fix not only the error type, but also every place you construct it (probably inserting a bunch of Box::new).
Hopefully you've only added one error variant, otherwise you have to also figure out which of the variants you just added is causing the error to go over the size limit.
There was a problem hiding this comment.
Fair points, but the added code is quite minimal (just adding Box and a new From implementation), and yes - printing the size is very useful. You wouldn't want to tell clement to just hover the type in vim (especially since it doesn't even work reliably in VSCode)
There was a problem hiding this comment.
Fair points, but the added code is quite minimal (just adding Box and a new From implementation)
I think you've underestimating how annoying it is for this to be a test. Two reviewers on this PR asked you to change it... We have precedent for types being checked against some constraints at compile-time, or code not compiling if you're not handling all struct fields/enum variants somewhere. With tests, at least personally I'll only find out about this issue after CI runs on my PR, adding another thing to do just to "make CI green".
You wouldn't want to tell clement to just hover the type in vim
I would, because he uses rust analyzer, like everyone else in this company. Hover information is not a VSCode specific thing.
(especially since it doesn't even work reliably in VSCode)
It is reliable. The only time it doesn't show the size is if the type does not have a size yet. None of these errors have type parameters, so they show the size just fine:
Here's what a generic type looks like:
And if you fill in the type:
There was a problem hiding this comment.
We do already depend on const_format in the repository, so using it for this wouldn't be adding a new dependency.
There was a problem hiding this comment.
I agree with Jan! rustanalyzer struct size reports have been reliable for me as well and anything that puts a failure from compile time to test time is a potential productivity loss
There was a problem hiding this comment.
Large errors leads to large
Results, which may lead to a lot ofmemcpy:s when returning aResult(even anOkone), which can hurt performance.Some of our error types were over 100 bytes, even 200 bytes.
Many of our errors are still pretty huge (64+ bytes), but this is a start.