Add size-optimized FFI functions for Locale and DataProvider #962

sffc · 2021-08-18T00:36:09Z

This PR copies the fixeddecimal example to use small-code functions.

optim5.elf is 56 KB after this change (60 KB if I keep the additional coverage tests).

coveralls · 2021-08-18T00:51:21Z

Pull Request Test Coverage Report for Build f4c807cf2c1cd4b83f44009de1a393a49a5e16ea-PR-962

65 of 87 (74.71%) changed or added relevant lines in 6 files are covered.
10 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.06%) to 75.031%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
utils/writeable/src/impls.rs	58	60	96.67%
ffi/capi/src/locale.rs	0	6	0.0%
ffi/capi/src/provider.rs	0	6	0.0%
ffi/capi/src/decimal.rs	0	8	0.0%

Files with Coverage Reduction	New Missed Lines	%
provider/blob/src/static_data_provider.rs	10	37.5%

Totals
Change from base Build 245eb95ee01b2cabdea611a2f1b8436b4ad7890b:	-0.06%
Covered Lines:	9679
Relevant Lines:	12900

💛 - Coveralls

Manishearth · 2021-08-18T01:35:17Z

ffi/capi/examples/fixeddecimal/test.c

-    ICU4XLocale* locale = ICU4XLocale_create("bn", 2);
-    ICU4XCreateDataProviderResult result = ICU4XDataProvider_create_static();
+    ICU4XLocale* locale = ICU4XLocale_create_bn();
+    ICU4XCreateFixedDecimalFormatDataProviderResult result = ICU4XFixedDecimalFormatDataProvider_create_static();


thought: Given these changes should we split this example into an entirely separate one? That way we can have a "normal" FDF example as a real example, and then the size optimized one can be as weird as it likes

jira-pull-request-webhook · 2021-08-18T06:32:01Z

Notice: the branch changed across the force-push!

.github/workflows/build-test.yml is now changed in the branch
Cargo.lock is different
ffi/capi/Cargo.toml is different
ffi/capi/examples/fixeddecimal/test.c is different
ffi/capi/include/ICU4XCreateFixedDecimalFormatDataProviderResult.h is no longer changed in the branch
ffi/capi/include/ICU4XCreateStaticDataProviderResult.h is now changed in the branch
ffi/capi/include/ICU4XFixedDecimalFormat.h is different
ffi/capi/include/ICU4XFixedDecimalFormatDataProvider.h is no longer changed in the branch
ffi/capi/include/ICU4XStaticDataProvider.h is now changed in the branch
ffi/capi/src/decimal.rs is different
ffi/capi/src/fixed_decimal.rs is now changed in the branch
ffi/capi/src/locale_canonicalizer.rs is now changed in the branch
ffi/capi/src/locale.rs is different
ffi/capi/src/pluralrules.rs is now changed in the branch
ffi/capi/src/provider.rs is now changed in the branch
ffi/cpp/docs/source/decimal_ffi.rst is now changed in the branch
ffi/cpp/docs/source/locale_ffi.rst is now changed in the branch
ffi/cpp/docs/source/provider_ffi.rst is now changed in the branch
ffi/cpp/include/ICU4XCreateFixedDecimalFormatDataProviderResult.hpp is no longer changed in the branch
ffi/cpp/include/ICU4XCreateStaticDataProviderResult.h is now changed in the branch
ffi/cpp/include/ICU4XCreateStaticDataProviderResult.hpp is now changed in the branch
ffi/cpp/include/ICU4XFixedDecimalFormat.h is different
ffi/cpp/include/ICU4XFixedDecimalFormat.hpp is different
ffi/cpp/include/ICU4XFixedDecimalFormatDataProvider.hpp is no longer changed in the branch
ffi/cpp/include/ICU4XLocale.hpp is different
ffi/cpp/include/ICU4XStaticDataProvider.h is now changed in the branch
ffi/cpp/include/ICU4XStaticDataProvider.hpp is now changed in the branch
ffi/wasm/docs/decimal_ffi.rst is now changed in the branch
ffi/wasm/docs/locale_ffi.rst is now changed in the branch
ffi/wasm/docs/provider_ffi.rst is now changed in the branch
ffi/wasm/lib/api.mjs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

jira-pull-request-webhook · 2021-08-18T06:44:40Z

Notice: the branch changed across the force-push!

.github/workflows/build-test.yml is no longer changed in the branch
ffi/capi/src/fixed_decimal.rs is no longer changed in the branch
ffi/capi/src/locale_canonicalizer.rs is no longer changed in the branch
ffi/capi/src/pluralrules.rs is no longer changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

codecov-commenter · 2021-08-18T06:44:59Z

Codecov Report

Merging #962 (6fea805) into main (245eb95) will decrease coverage by 0.06%.
The diff coverage is 74.71%.

@@            Coverage Diff             @@
##             main     #962      +/-   ##
==========================================
- Coverage   75.05%   74.99%   -0.07%     
==========================================
  Files         215      216       +1     
  Lines       12652    12732      +80     
==========================================
+ Hits         9496     9548      +52     
- Misses       3156     3184      +28

Impacted Files	Coverage Δ
ffi/capi/src/decimal.rs	`0.00% <0.00%> (ø)`
ffi/capi/src/locale.rs	`0.00% <0.00%> (ø)`
ffi/capi/src/provider.rs	`0.00% <0.00%> (ø)`
utils/writeable/src/lib.rs	`90.00% <ø> (ø)`
utils/writeable/src/impls.rs	`96.66% <96.66%> (ø)`
provider/blob/src/path_util.rs	`100.00% <100.00%> (ø)`
provider/core/src/resource.rs	`82.77% <100.00%> (+0.74%)`	⬆️
provider/blob/src/static_data_provider.rs	`37.50% <0.00%> (-17.05%)`	⬇️
utils/fixed_decimal/src/decimal.rs	`94.69% <0.00%> (+0.01%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 245eb95...6fea805. Read the comment docs.

…ement

sffc · 2021-08-18T08:05:19Z

The CI error is

  = note: /usr/bin/ld: error: LLVM gold plugin has failed to create LTO module: Invalid record
          clang: error: linker command failed with exit code 1 (use -v to see invocation)

I cannot reproduce this locally on the specified toolchain version (nightly-2021-02-28), although I can reproduce it on other toolchain versions like nightly-2021-08-18.

I suspect the issue is most likely a different version of lld on CI than I have on my local computer. According to rust-lang/rust#60059 (comment), it is necessary to use a specific version of lld found in Rust's fork of LLVM.

Should I:

Keep trying to get a build running in CI
Remove the CI for now and work on it in a follow-up

Manishearth · 2021-08-18T16:04:41Z

  = note: /usr/bin/ld: error: LLVM gold plugin has failed to create LTO module: Invalid record
          clang: error: linker command failed with exit code 1 (use -v to see invocation)

in test-capi-tiny

sffc · 2021-08-18T16:08:28Z

  = note: /usr/bin/ld: error: LLVM gold plugin has failed to create LTO module: Invalid record
          clang: error: linker command failed with exit code 1 (use -v to see invocation)

in test-capi-tiny

Thoughts on the question I posed in my previous post?

Manishearth · 2021-08-18T16:11:37Z

I'd remove the CI for now and document the specific steps.

sffc requested a review from Manishearth August 18, 2021 00:36

Manishearth previously approved these changes Aug 18, 2021

View reviewed changes

sffc dismissed Manishearth’s stale review via 71ceb73 August 18, 2021 06:31

sffc force-pushed the codesize2 branch from 1e14675 to 71ceb73 Compare August 18, 2021 06:31

sffc added 8 commits August 18, 2021 01:44

Eliminate low-hanging fruit for code size of FFI example

2032a4b

Add generated Diplomat files

64616f2

Add initial ICU4XStaticDataProvider

e6a1e30

Update Diplomat bindings

09cf5c5

cargo fmt

319dc34

Update test.c

6c5a1ed

Move size-optimized example to another directory

40a150c

Revert test.c to mainline

6dca1d1

sffc force-pushed the codesize2 branch from e997295 to 6dca1d1 Compare August 18, 2021 06:44

sffc added 3 commits August 18, 2021 01:48

Remove additional coverage tests from fixeddecimal_tiny

2bb5c46

Add test-capi-tiny

f51d0d1

Add LLVM tooling to build-test.yml

703e3b1

sffc requested a review from Manishearth August 18, 2021 06:57

sffc marked this pull request as ready for review August 18, 2021 06:57

sffc requested a review from a team as a code owner August 18, 2021 06:57

sffc changed the title ~~Low-hanging fruit for code size improvements~~ Add size-optimized FFI functions for Locale and DataProvider Aug 18, 2021

sffc added 4 commits August 18, 2021 02:10

Set a stable nightly toolchain version for FFI tests

df8d136

Attempt to fix build issue by reverting to nightly-2021-02-28

19b6309

Rewrite resource_path_to_string for a small code size improvement

fd18830

Add Writeable impls for u8 and u16 for another small code size improv…

763efbb

…ement

cargo fmt

170b4aa

Manishearth previously approved these changes Aug 18, 2021

View reviewed changes

Temporarily disable test-capi-tiny on CI

6fea805

sffc dismissed Manishearth’s stale review via 6fea805 August 18, 2021 17:16

sffc requested a review from Manishearth August 18, 2021 17:16

Manishearth approved these changes Aug 18, 2021

View reviewed changes

sffc merged commit d320b04 into unicode-org:main Aug 18, 2021

sffc deleted the codesize2 branch August 18, 2021 18:35

sffc linked an issue Aug 18, 2021 that may be closed by this pull request

Determine low-hanging fruit in FixedDecimalFormat FFI example #951

Closed

sffc mentioned this pull request Aug 18, 2021

Determine low-hanging fruit in FixedDecimalFormat FFI example #951

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add size-optimized FFI functions for Locale and DataProvider #962

Add size-optimized FFI functions for Locale and DataProvider #962

sffc commented Aug 18, 2021 •

edited

Loading

coveralls commented Aug 18, 2021 •

edited

Loading

Manishearth Aug 18, 2021

jira-pull-request-webhook bot commented Aug 18, 2021

jira-pull-request-webhook bot commented Aug 18, 2021

codecov-commenter commented Aug 18, 2021 •

edited

Loading

sffc commented Aug 18, 2021

Manishearth commented Aug 18, 2021

sffc commented Aug 18, 2021

Manishearth commented Aug 18, 2021

Add size-optimized FFI functions for Locale and DataProvider #962

Add size-optimized FFI functions for Locale and DataProvider #962

Conversation

sffc commented Aug 18, 2021 • edited Loading

coveralls commented Aug 18, 2021 • edited Loading

Pull Request Test Coverage Report for Build f4c807cf2c1cd4b83f44009de1a393a49a5e16ea-PR-962

💛 - Coveralls

Manishearth Aug 18, 2021

Choose a reason for hiding this comment

jira-pull-request-webhook bot commented Aug 18, 2021

jira-pull-request-webhook bot commented Aug 18, 2021

codecov-commenter commented Aug 18, 2021 • edited Loading

Codecov Report

sffc commented Aug 18, 2021

Manishearth commented Aug 18, 2021

sffc commented Aug 18, 2021

Manishearth commented Aug 18, 2021

sffc commented Aug 18, 2021 •

edited

Loading

coveralls commented Aug 18, 2021 •

edited

Loading

codecov-commenter commented Aug 18, 2021 •

edited

Loading