Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Reduce stack size of DecimalSymbolsV1 #4437

Closed
sffc opened this issue Dec 9, 2023 · 5 comments · Fixed by #5804
Closed

Reduce stack size of DecimalSymbolsV1 #4437

sffc opened this issue Dec 9, 2023 · 5 comments · Fixed by #5804
Labels
A-data Area: Data coverage or quality C-numbers Component: Numbers, units, currencies
Milestone
ICU4X 2.0 Stretch...

Comments

@sffc
Copy link
Member

sffc commented Dec 9, 2023

Currency DecimalSymbolsV1 is 192 bytes. We should consider reducing that size since it is retained in a lot of other types, so we get a high ROI by reducing it. Here's a sandbox where I got it down to 56 bytes without sacrificing much:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=5900f3431d428cb8b11695b6ca9dbeea

Other representations may be possible as well.
@sffc sffc added C-numbers Component: Numbers, units, currencies A-data Area: Data coverage or quality labels Dec 9, 2023
@sffc sffc added this to the ICU4X 2.0 milestone Dec 9, 2023
@Manishearth Manishearth moved this to Unclaimed for sprint in icu4x 2.0 Feb 23, 2024
@Manishearth
Copy link
Member

One downside of this would be that the Pixel Watch code, which directly constructs a DecimalSymbolsV1, would have to resort to manually constructing a VarZeroVec, which we try not to make a runtime problem.

@sffc
Copy link
Member Author

sffc commented Mar 29, 2024

Here's a 112-byte version with no VarZeroVecs

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=317921850e317c266ded75b8432422a3

@sffc sffc added the needs-approval One or more stakeholders need to approve proposal label Mar 29, 2024
@sffc sffc moved this from Unclaimed for sprint to Needs discussion to unblock in icu4x 2.0 Mar 29, 2024
@sffc
Copy link
Member Author

sffc commented Mar 29, 2024

DecimalSymbolsV3 is probably just about the same performance-wise as DecimalSymbolsV1. VarZeroVecs add runtime cost for lookup.

@Manishearth Manishearth removed the needs-approval One or more stakeholders need to approve proposal label May 9, 2024
@Manishearth Manishearth moved this from Needs discussion to unblock to Unclaimed for sprint in icu4x 2.0 May 9, 2024
@sffc
Copy link
Member Author

sffc commented May 9, 2024

  • @Manishearth - We should look at postcard size, too. And measure performance. But initially version 2 seems good.

@sffc sffc mentioned this issue Jul 17, 2024
Closed
@sffc sffc mentioned this issue Aug 19, 2024
Closed
@sffc sffc mentioned this issue Sep 23, 2024
Merged
@Manishearth
Copy link
Member

@sffc your design here would be optimized by the new MultiFieldsULE thing too, if we added ArrayVarULE

@Manishearth Manishearth mentioned this issue Nov 6, 2024
Merged
Manishearth added a commit that referenced this issue Nov 7, 2024
@Manishearth
Add TupleNVarULE (#5777)
4522f5b 
The VarULE counterpart of TupleNULE

Part of #5523. Planned to be
used in #4437

I'm not super happy with the naming with this vs VarTupleULE, but I've
tried to make it clearer with the module names and it's fine for now. We
can rename as desired since zerovec isn't on the ICU4X stability track.

I do plan to add serde/etc impls but that's going to be a separate PR.


<!--
Thank you for your pull request to ICU4X!

Reminder: try to use [Conventional
Comments](https://conventionalcomments.org/) to make comments clearer.

Please see
https://github.com/unicode-org/icu4x/blob/main/CONTRIBUTING.md for
general
information on contributing to ICU4X.
-->
@Manishearth Manishearth mentioned this issue Nov 12, 2024
Merged
@sffc sffc mentioned this issue Nov 12, 2024
Merged
sffc added a commit that referenced this issue Nov 12, 2024
@sffc
Refactor size_test_macro and use it in icu_decimal (#5805)
09fde64 
Test for #4437
@github-project-automation github-project-automation bot moved this from Unclaimed for sprint to Done in icu4x 2.0 Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-data Area: Data coverage or quality C-numbers Component: Numbers, units, currencies
Projects
icu4x 2.0
Status: Done
Milestone
ICU4X 2.0 Stretch ⟨P2⟩
Development

Successfully merging a pull request may close this issue.

Reduce stack size of DecimalSymbolsV1 Manishearth/icu4x
2 participants
@sffc @Manishearth