Skip to content

Use musl implementation of strftime rather than custom JS code. NFC #21379

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

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Feb 20, 2024

This leads to some code size savings in several tests, reduces the differences between standalone mode and non-standalone mode and reduces the amount of custom JS code we need to maitain.

In simple microbenchmark that does nothing but export strftime we do code size win at -O0 but a regression in codesize at -O2:

$ wc -c old_O0.*
74026 old.js
12124 old.wasm
86150 total
$ wc -c new_O0.*
62120 new.js
22852 new.wasm
84972 total
$ wc -c old_O2.*
8492 old.js
 194 old.wasm
8686 total
$ wc -c new_O2.*
 5040 new.js
14291 new.wasm
19331 total

This regression comes from the fact that snprintf is used in the implementation of strftime and its hard to optimize away the cost of that function.

In practice I suspect that any application large enough to call strftime is likely already directly or indirectly pulling in snprintf so I would hope that this will be codesize win in practice since it removes about 3k of JS.

@sbc100 sbc100 requested review from kripken and dschuff February 20, 2024 19:17
@sbc100 sbc100 changed the title Use musl implementation of strftime rather than custom JS one. NFC Use musl implementation of strftime rather than custom JS code. NFC Feb 20, 2024
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Agree that likely in real-world cases this shrinks code size, and is a very nice simplification.

@dschuff
Copy link
Member

dschuff commented Feb 20, 2024

Agreed, I think this is worth it.

@sbc100
Copy link
Collaborator Author

sbc100 commented Feb 20, 2024

I had some second thoughts here because I discovered that C++ (specifically C++ i/o streams) end up depending on strftime. So this could effect more programs that I initially expected.

@sbc100 sbc100 force-pushed the musl_strftime branch 2 times, most recently from 1e707e3 to 0292054 Compare June 26, 2024 17:40
@sbc100 sbc100 enabled auto-merge (squash) June 26, 2024 18:05
This leads to some code size savings in several tests, reduces the
differences between standalone mode and non-standalone mode and reduces
the amount of custom JS code we need to maitain.

In simple microbenchmark that does nothing but export `strftime` we do
code size win at `-O0` but a regression in codesize at `-O2`:

```
$ wc -c old_O0.*
74026 old.js
12124 old.wasm
86150 total
$ wc -c new_O0.*
62120 new.js
22852 new.wasm
84972 total
```

```
$ wc -c old_O2.*
8492 old.js
 194 old.wasm
8686 total
$ wc -c new_O2.*
 5040 new.js
14291 new.wasm
19331 total
```

This regression comes from the fact that `snprintf` is used in the
implementation of `strftime` and its hard to optimize away the cost of
that function.

In practice I suspect that any application large enough to call
`strftime` is likely already directly or indirectly pulling in
`snprintf` so I would hope that this will be codesize win in practice
since it removes about 3k of JS.
@sbc100 sbc100 merged commit addff4f into emscripten-core:main Jun 26, 2024
28 checks passed
@sbc100 sbc100 deleted the musl_strftime branch June 26, 2024 19:18
HeavenVolkoff added a commit to HeavenVolkoff/archive-wasm that referenced this pull request Aug 1, 2024
 - Patch missing __secs_to_zone, due to emscripten-core/emscripten#21379
 - Better fix for tuklib_integer.cmake usage error in liblzma when compiling with emscripten
 - Fix lzma test failure
HeavenVolkoff added a commit to HeavenVolkoff/archive-wasm that referenced this pull request Aug 2, 2024
 - Patch missing __secs_to_zone, due to emscripten-core/emscripten#21379
 - Better fix for tuklib_integer.cmake usage error in liblzma when compiling with emscripten
 - Fix lzma test failure
sbc100 added a commit to sbc100/emscripten that referenced this pull request Aug 2, 2024
sbc100 added a commit to sbc100/emscripten that referenced this pull request Aug 2, 2024
sbc100 added a commit to sbc100/emscripten that referenced this pull request Aug 3, 2024
sbc100 added a commit that referenced this pull request Aug 3, 2024
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.

3 participants