Skip to content

Reducing size of DateTimeFormatter and other large objects #3413

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

Closed
4 tasks done
sffc opened this issue May 5, 2023 · 11 comments
Closed
4 tasks done

Reducing size of DateTimeFormatter and other large objects #3413

sffc opened this issue May 5, 2023 · 11 comments
Assignees
Labels
C-data-infra Component: provider, datagen, fallback, adapters C-datetime Component: datetime, calendars, time zones

Comments

@sffc
Copy link
Member

sffc commented May 5, 2023

DateTimeFormatter is 6.6 KB on the stack. This is because it owns a lot of DataPayloads, which are themselves collections of pointers to strings either in bake data or in a postcard buffer. In total, there are hundreds of strings in DateTimeFormatter that may or may not be present in a particular instance.

This is more impactful in C++ than in Rust, because we put the DateTimeFormatter on the heap when returning it to C++.

A few options:

  1. Ignore. C++ users need more heap memory.
  2. Add a global feature on icu_provider that makes DataPayload be much smaller in the BakedDataProvider case, and allocating the existing yoke in the BufferProvider case. The BakedDataProvider will not call ZeroFrom, instead leaving just a &'static reference.
    • Pro: Easy to configure for clients who only care about bake data.
    • Con: Postcard clients take a hit if this mode is enabled.
    • Con: Not great to use with --all-features
  3. Add a type parameter to all formatters that causes them to either have large stack or small stack. The type parameter would have a default value for the status quo behavior (large stack). All formatters would then gain try_new_with_static_provider which would return the formatter in small-stack mode. BakedDataProvider would implement a trait that can be used with this new function.
    • Pro: No global behavior-switching feature flag
    • Pro: Code for optimal Postcard and Baked usage can co-exist
    • Con: Much more complicated in the type system; we are already pushing the bounds of the type system
    • Con: Might negatively affect code size; would need to measure impact
  4. Other?

CC @Manishearth @robertbastian


Update (29-06-2023)

Sub-issues:

Pre-2.0:

Post-2.0:

@sffc sffc added C-data-infra Component: provider, datagen, fallback, adapters discuss Discuss at a future ICU4X-SC meeting labels May 5, 2023
@Manishearth
Copy link
Member

Manishearth commented May 5, 2023

Two other options that came up during discussion:

  1. ZeroBox: We introduce a type ZeroBox/VarZeroBox which has the semantics of a boxing Cow (by default Cow goes to the ToOwned type). It requires the entire data behind it to be ULE. We sprinkle it in on large stack size data payloads. Tradeoffs:
    • Pro: No global flag
    • Pro: Can be targeted
    • Pro: does not disadvantage postcard over baked
    • Con: not a perfect win for Baked since the untargeted cases still take up stack space
    • Con: Probably slower as it'll go through ULE machinery
    • Con: lots and lots of annoying work making everything ULE or VarULE.
    • Con: needs try_new_with_static_provider()
  2. DataPayloadCow: We introduce a type DataPayloadCow that has the semantics of a boxing Cow around DataPayload.
    • Pro: can be targeted
    • Pro: no global flag
    • Pro: no need to ULE everything
    • Con: forces allocations on postcard each time
    • Con: still an imperfect win for Baked
    • Con: needs try_new_with_static_provider()

@sffc
Copy link
Member Author

sffc commented May 5, 2023

In any of these modes that involves boxing the data struct for postcard users, it would be nice if they could work with a cache of deserialized postcard objects (the cache owns the heap-allocated objects and the formatters reference them out of the cache). Haven't thought through how it would work exactly.

@sffc
Copy link
Member Author

sffc commented May 11, 2023

Discuss with:

@Manishearth
Copy link
Member

Related: #3020

@robertbastian
Copy link
Member

robertbastian commented May 25, 2023

core::mem::size_of::<alloc::borrow::Cow<str>>() is 32 on stable and 24 on nightly. That's nice, but maybe we can get it down to 16 if we make the owned type Box<str> instead of String and find some niche.

@sffc sffc added the discuss-triaged The stakeholders for this issue have been identified and it can be discussed out-of-band label May 25, 2023
@sffc
Copy link
Member Author

sffc commented May 25, 2023

I don't see a way to get to 16 for a cowable slice. In the non-null case, both the borrowed and the owned variants need two full usizes. The only niche is when the pointer is exactly 0, which is sufficient for Option but not when both variants need 16 bits of storage. So, since we need to overflow into a third word, we may as well keep the capacity.

@robertbastian
Copy link
Member

robertbastian commented May 26, 2023

We could for example make the assumption that our strings will at most be half a usize long.

@sffc
Copy link
Member Author

sffc commented May 26, 2023

Maybe, but then we technically need fallible conversions from Box<str> and &str into HalfLengthCowStr. Or maybe that's enough of a degenerate case that we can debug-assert and GIGO.

@Manishearth
Copy link
Member

Manishearth commented Jun 29, 2023

  • @sffc I think it's a no brainer to try specifically fixing DateTimeFormat on the pre-2.0 timeline. Fancy cross-cutting things like ZeroBox/etc are maybe not worth it
  • @Manishearth Do we have other things as large as DateTimeFormat?
  • @sffc not even close. we should write a test in verify-zero-copy or in fingerprint

Talking about map_project issue

  • @Manishearth in some cases may be able to introduce map_project_maybe_ref(|x| -> y, |&x| -> &y)
  • @sffc could also introduce a DataPayloadOnlyYoke that avoids the staticref problem
  • @sffc in datetime we only use map_project for patterns
  • @sffc Would be much simpler to store patterns in a small stack with heap overflow. So we don't need map_project anymore
  • When @eggrobin does his refactor; reduce map_projects or make them less impactful

Options for fixing datetimeformat size itself:

  1. Make SolarTwelve a VZV. Maybe also make the other things ZeroVecs. Can be done backwards compatibly but will make old-data-new-code (postcard) cause allocs.
  2. Split symbols further into tiny pieces, one DTF object only needs a small subset of symbols. How does the data loading work??? @eggrobin is somewhat skeptical about this actually reducing size too much due to complex dependencies
  3. Use PatternInterpolator design to making a billion datetimeformat objects, this would imply one per locale (and multiple interpolators per locale each of which is small)
  4. Use some funky DataPayload-derived type in DTF
    • DataPayload gets APIs that expose its guts
    • DateTimeFormat instead of containing a DataPayload (for symbols) will contain something that is effectively an enum between Box<Yoke> and &'static Symbols. A potential future no-alloc mode would omit the box.
    • Can be done backcompat-ily
    • If we split the data later we can do some weird enumy things

@sffc For option 1 it's a bunch of tricky custom deser impls, maybe just wait for 2.0 and Better Sliced Symbols.

Let's do number 4 (new data payload) and we're doing number 3 anyway (pattern interpolator). We can do Numbers 1 and 2 in the 2.0 timeframe. Number 2 needs investigation.

Decision: @sffc @Manishearth @younies @robertbastian @skius (@eggrobin 😕)

Milestones: @sffc hoping to get the new pattern interpolator done in 1.4. New data payload thingy should be Q3, 1.x Priority.

manish to file issues

@robertbastian robertbastian removed discuss Discuss at a future ICU4X-SC meeting discuss-triaged The stakeholders for this issue have been identified and it can be discussed out-of-band labels Feb 28, 2024
@sffc sffc added the C-datetime Component: datetime, calendars, time zones label Feb 29, 2024
@sffc sffc added this to the 1.5 Blocking ⟨P1⟩ milestone Feb 29, 2024
@sffc sffc self-assigned this Feb 29, 2024
@sffc
Copy link
Member Author

sffc commented May 16, 2025

All the sub-issues are done, and the 2.0 DateTimeFormatter (YMD mode) is 384 bytes, a 94% improvement from 1.5. So I am going to mark this as fixed.

@sffc sffc closed this as completed May 16, 2025
@sffc sffc removed this from the 2.x Priority ⟨P2⟩ milestone May 16, 2025
@sffc sffc added this to the ICU4X 2.0 ⟨P1⟩ milestone May 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-data-infra Component: provider, datagen, fallback, adapters C-datetime Component: datetime, calendars, time zones
Projects
None yet
Development

No branches or pull requests

3 participants