Optimize size of TzOffset
#165
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
djc
reviewed
Apr 25, 2024
There was a problem hiding this comment.
IMO the integers containing bitshifted fields should be wrapped in a newtype with a const API that can handle both construction and getting out the individual values.
Instead of simply concatenating all abbrevations, how much smaller does the full string get if you concatenate only abbreviations that aren't already in the earlier string?
| let mut abbreviations: Vec<_> = abbreviations.iter().collect(); | ||
| abbreviations.sort(); | ||
| let mut abbreviations_str = String::new(); | ||
| for abbr in abbreviations.drain(..) { |
There was a problem hiding this comment.
I think this is abbrevations.join("")?
chrono-tz-build/src/lib.rs
Outdated
| @@ -208,16 +225,17 @@ impl FromStr for Tz {{ | |||
| first: FixedTimespan {{ | |||
| utc_offset: {utc}, | |||
| dst_offset: {dst}, | |||
| name: \"{name}\", | |||
| abbreviation: {index_len}, | |||
There was a problem hiding this comment.
Let's suffix the field name with _idx?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Three changes that bring the size of
DateTime<Tz>down from 48 bytes to 20 bytes:i16. All abbreviations are at most 6 characters and the entire string is ca. 650 characters. So it fits easily.i16, it needs 17 bits. We give it 18 bits of ani32. The DST offset from the base offset is much smaller, it can fit into the remaining 14 bits. This spares onei32.FixedTimespantype, and 2 bytes of padding in theTzOffsettype. If we don't wrap theFixedTimespanbut copy its two fields intoTzOffsetwe get rid of the padding, which is 33% of the type.A binary compiled with these size optimizations is 1,5Mb smaller 🎉.
Fixes #27.