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.

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

Stabilize rounding to increment methods for FixedDecimal #4578

Merged
merged 10 commits into from
Feb 12, 2024

Conversation

jedel1043
Copy link
Contributor

Follow up of #4571.

@jedel1043 jedel1043 requested review from sffc, Manishearth and a team as code owners February 3, 2024 07:16
@jedel1043
Copy link
Contributor Author

The failing action seems to be sporadic; I ran it locally without problems.

@sffc
Copy link
Member

sffc commented Feb 5, 2024

Note: No binsize diff.

Main:

-rwxr-xr-x 1 root root    24248 Feb  5 23:27 optim4.elf
-rwxr-xr-x 1 root root    13520 Feb  5 23:27 optim5.elf

PR:

-rwxr-xr-x 1 root root    24256 Feb  5 14:00 optim4.elf
-rwxr-xr-x 1 root root    13520 Feb  5 14:00 optim5.elf

@sffc
Copy link
Member

sffc commented Feb 5, 2024

https://github.com/unicode-org/icu4x/blob/main/docs/tutorials/c-tiny/fixeddecimal/test.c doesn't include calls to rounding functions. I think we should probably add that.

@sffc
Copy link
Member

sffc commented Feb 6, 2024

On main:

-rwxr-xr-x 1 root root    25568 Feb  6 16:43 optim4.elf
-rwxr-xr-x 1 root root    14464 Feb  6 16:43 optim5.elf

On this branch:

-rwxr-xr-x 1 root root    29112 Feb  6 01:16 optim4.elf
-rwxr-xr-x 1 root root    17520 Feb  6 01:16 optim5.elf

🐖

@sffc
Copy link
Member

sffc commented Feb 6, 2024

@jedel1043 I think we can use generics to solve this and stay DRY. Sandbox example:

https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=c9c7c4788888d36fc3e1b37e7b9dd36a

Build it as WASM and check the output. Note that trunc and trunc_to_increment compile to different code, with trunc being substantially smaller than trunc_to_increment.

This will cause duplicated code between the increment and non-increment functions, but I think this is okay because if you care about code size you should just use either one or the other, and we know that many users don't need increments so we don't want to penalize those callers.

@jedel1043
Copy link
Contributor Author

jedel1043 commented Feb 7, 2024

Did a size comparison of all possible solutions:

main

-rwxr-xr-x 1 jedel users    25376 Feb  6 23:19 optim4.elf
-rwxr-xr-x 1 jedel users    14728 Feb  6 23:19 optim5.elf

RoundingIncrement

-rwxr-xr-x 1 jedel users    28768 Feb  6 23:27 optim4.elf
-rwxr-xr-x 1 jedel users    17656 Feb  6 23:27 optim5.elf

IncrementLike

-rwxr-xr-x 1 jedel users    26128 Feb  7 00:25 optim4.elf
-rwxr-xr-x 1 jedel users    15000 Feb  7 00:25 optim5.elf

IncrementLike + #[inline(never)] (current)

-rwxr-xr-x 1 jedel users    25648 Feb  7 00:39 optim4.elf
-rwxr-xr-x 1 jedel users    14920 Feb  7 00:39 optim5.elf

Adding #[inline] to all the internal methods barely changes the size of optim4.elf (16 bytes) and doesn't change the size of optim5.elf.

sffc
sffc previously approved these changes Feb 7, 2024
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making this change! This looks good to me now to land. After it lands we can play around a little more with the #[inline] attributes to measure the impact.

I'm going to copy @markusicu on this PR in order to grant the stabilization review.

@sffc sffc requested a review from markusicu February 7, 2024 18:51
Copy link
Member

@robertbastian robertbastian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please wait for @markusicu review as well

@sffc
Copy link
Member

sffc commented Feb 12, 2024

Notes from the API Review:

https://docs.google.com/document/d/1gKZCaiTG2eeyxrIKUF2tf6PIsaPCIBaUVRXD-JDh6dI/edit#heading=h.7pm5mstzwo40

@markusicu left numerous suggestions about documentation and generally the FixedDecimal API shape which we can fix in the 2.0 timeframe. I will file issues about those. However, @markusicu was happy with the shape and naming of RoundingIncrement and *_to_increment, so we have the i18n Correctness approval to land the new API.

Thanks @jedel1043!

@Manishearth Manishearth merged commit 9c1d923 into unicode-org:main Feb 12, 2024
30 checks passed
@jedel1043 jedel1043 deleted the stabilize-rounding-increments branch February 12, 2024 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants