-
-
Notifications
You must be signed in to change notification settings - Fork 272
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
Shrink DecodeError
from 48 to 32 bytes on 64-bit arch
#553
Conversation
Codecov Report
@@ Coverage Diff @@
## trunk #553 +/- ##
==========================================
+ Coverage 54.44% 54.49% +0.05%
==========================================
Files 49 50 +1
Lines 4423 4426 +3
==========================================
+ Hits 2408 2412 +4
+ Misses 2015 2014 -1
Continue to review full report at Codecov.
|
This comment was marked as outdated.
This comment was marked as outdated.
Actually I think I miscalculated, and we can get 32 bytes wide Gonna redo that. |
Can we add a test to validate the size of all |
FYI: this is the variant that makes /// The encoder tried to encode a `SystemTime`, but it was before `SystemTime::UNIX_EPOCH`
#[cfg(feature = "std")]
InvalidSystemTime {
/// The error that was thrown by the SystemTime
inner: std::time::SystemTimeError,
/// The SystemTime that caused the error
time: std::time::SystemTime,
}, Could remove |
Interesting, on But why |
Kind of makes sense, I've boxed |
Oh I thought we were compiling against Boxing the SystemTime seems like a good solution to me |
Thanks! |
This is done by two tricks:
entire description of an enum with it's type name andallowed variants behind a&'static
ref.DecodeError::CStringNulError
to just the position of the null byte.Aside from the extra struct being added, this is a breaking change due to type names for many manual impls being reduced to just the identifier name (derived impls were already just the identifier here). This is unfortunately necessary asstd::any::type_name
is notconst
, and even if it were using it with an external generic type would make it unusable in const environment, meaning it's impossible to construct a staticAllowedEnum
struct with it.CStringNulError
I think is exotic enough and rare enough that dropping information here makes sense, if not we could alsoBox
the internal error (std already allocates there anyway).If these changes are unacceptable, it's still possible to reduce the size ofDecodeError
to 40 bytes by just putting theAllowedEnumVariants
behind a&'static
ref, as that enum alone is 3 words wide.