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

Coalesce more impls into the new BufferProvider framework #1384

Merged
merged 3 commits into from
Dec 10, 2021

Conversation

sffc
Copy link
Member

@sffc sffc commented Dec 10, 2021

Progress on #1107
See #837
See #1246

I wanted to do this as a separate PR so I could measure impact on code size. In the main code size example (fixeddecimal_tiny), making this change results in a very small code size increase; the maintainability and flexibility of the new approach is well worth the tradeoff, in my opinion.

@sffc sffc requested a review from a team as a code owner December 10, 2021 10:20
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

I love this, and this means that multiple providers over FFI is no longer a codesize hazard!

@sffc
Copy link
Member Author

sffc commented Dec 10, 2021

We can compare code size based on CI now 😁

On main:

-rwxr-xr-x 1 runner docker 32261040 Dec 10 10:26 optim0.elf
-rwxr-xr-x 1 runner docker   755984 Dec 10 10:26 optim1.elf
-rwxr-xr-x 1 runner docker  1962408 Dec 10 10:28 optim2.elf
-rwxr-xr-x 1 runner docker  2914296 Dec 10 10:28 optim3.elf
-rwxr-xr-x 1 runner docker    97760 Dec 10 10:29 optim4.elf
-rwxr-xr-x 1 runner docker    58768 Dec 10 10:29 optim5.elf

On this PR:

-rwxr-xr-x 1 runner docker 32179056 Dec 10 10:29 optim0.elf
-rwxr-xr-x 1 runner docker   977168 Dec 10 10:29 optim1.elf
-rwxr-xr-x 1 runner docker  1568432 Dec 10 10:30 optim2.elf
-rwxr-xr-x 1 runner docker  2412928 Dec 10 10:30 optim3.elf
-rwxr-xr-x 1 runner docker    99848 Dec 10 10:31 optim4.elf
-rwxr-xr-x 1 runner docker    60472 Dec 10 10:31 optim5.elf

I consider the change small enough to be in the noise. Looking at the IR, the main size difference is Rust thinking it needs to call some additional destructors given the one extra layer of indirection.

I'm eager to check this in, and consider a follow-up to improve code size for static data (perhaps in coordination with #1290).

@sffc sffc merged commit 0a9aea9 into unicode-org:main Dec 10, 2021
@sffc sffc deleted the rm-static branch December 10, 2021 23:40
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.

None yet

2 participants