Use musl's strptime implementation #22158
Conversation
sbc100
force-pushed
the
musl_strptime
branch
5 times, most recently
from
0140332 to
6d57da2
Compare
sbc100
force-pushed
the
musl_strptime
branch
3 times, most recently
from
8cce466 to
5649b70
Compare
That looks like a regression, not a savings, or are the two sentences talking about different things? |
Yeah sorry, it was a saving, but then once I added all that new code (the stuff that musl doesn't implement) it grew by a little and now its a slight regression., but its only 2%. |
This is a slight regression in code size compared the JS version, but makes the wasm file more standalone and portable. I measured the combined size of `core2.test_strptime_days` (with closure compiler enabled) and got the following results, which is a 2% regression: - before: 16777 bytes - after: 17235 bytes As part of this I updated musl's implementation to support several features that out current tests depend on: - Add support for '%z' for parsing timezone offsets. - Add support for '%U' and '%W' (week of the year) - Derive and set tm_yday/tm_mday/tm_yday fields where possible. Regarding '%U' and '%W' it seems this is missing from both musl and bionic: - https://github.com/aosp-mirror/platform_bionic/blob/4cfb2bde90fcf0a1f835e2b670b00e9f226c1c7c/libc/tzcode/strptime.c#L364-L375 - https://github.com/bminor/musl/blob/f314e133929b6379eccc632bef32eaebb66a7335/src/time/strptime.c#L126-L132 However, I implemented here (based on code from FreeBSD) in order to avoid any regressions (since we do have test coverage for these flags). Fixes: emscripten-core#21024
| * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) | ||
| * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE | ||
| * POSSIBILITY OF SUCH DAMAGE. | ||
| */ |
There was a problem hiding this comment.
These licenses look like 2-part BSD, which is different from musl and Emscripten's MIT license. We'd need to mention that in our main LICENSE file (which so far has only had to talk about MIT).
This isn't a big deal, I think, but given this also increases code size I am a little unsure of the benefit here. The code being more standalone is helpful, but maybe there are other ways to achieve that?
(Does WASI not have APIs to get the day of the week and stuff like that, like the Web does? If it does then perhaps we can farm out to Web/WASI APIs rather than bundle this code.)
There was a problem hiding this comment.
Another option is to remove all these additions, which would:
(1) + remove the license stuff
(2) + make this change a code size win
(3) - regress out support for certain formatting chars (to the same level as musl).
Regarding WASI APIs, no I don't think that would be needed since there is nothing system dependent going on here, we are just translating data from one format to another.
There was a problem hiding this comment.
Regarding the license, yes that is annoying I forgot we don't already have BSD stuff in out stdlib..
This is a less impactful version of emscripten-core#22158
This is a less impactful version of emscripten-core#22158
This is a less impactful version of #22158
This is a slight regression in code size compared the JS version, but makes
the wasm file more standalone and portable. I measured the combined size of
core2.test_strptime_days(with closure compiler enabled) and got thefollowing results, which is a 2% regression:
As part of this I updated musl's implementation to support several
features that our current tests depend on:
Regarding '%U' and '%W' it seems this is missing from both musl and
bionic:
However, I implemented here (based on code from FreeBSD) in order to
avoid any regressions (since we do have test coverage for these flags).
Fixes: #21024