Summary: | [JSC] Cache UDateTimePatternGenerator | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||
Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | ap, ews-watchlist, keith_miller, mark.lam, msaboff, ross.kirsling, saam, tzagallo, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 213425 | ||||||
Attachments: |
|
Description
Yusuke Suzuki
2020-06-22 01:40:54 PDT
We have a cache for encoding, does anything else show on profiles? (In reply to Alexey Proskuryakov from comment #1) > We have a cache for encoding, does anything else show on profiles? This is not related to encoding, this is for JSC Intl features including Intl.DateTimeFormat, NumberFormat etc. (In reply to Yusuke Suzuki from comment #2) > (In reply to Alexey Proskuryakov from comment #1) > > We have a cache for encoding, does anything else show on profiles? > > This is not related to encoding, this is for JSC Intl features including > Intl.DateTimeFormat, NumberFormat etc. For example, every time Intl.DateTimeFormat is created, we open and close UDateTimePatternGenerator, which involves ICU locale parsing. https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp#L608-L620 And further more, this code can be called every time we call `new Date().toLocaleString()` is called. https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/builtins/DatePrototype.js#L78 Furthermore, to make the code spec compliant, we need to generate ICU date-format pattern from "jjmm" skeleton to obtain hourCycleDefault (hcDefault in the spec). But this is per-locale 4-value enum. Getting dateformat generator from locale, passing skeleton to generate pattern, and parsing the pattern to obtain this 4-value enum every time we call is toLocaleString not so efficient. Currently, we are not doing this since it is costly. But we should to make our implementation spec compliant and caching is the best idea to avoid the above thing since it is per-locale thing. As the result, if you run for (var i = 0; i < 1e4; ++i) { new Date().toLocaleString(); } 80 - 90% of time is used for getting UDateTimePatternGenerator. If you add the other processing which can be avoided by caching, this part takes 95%. And the above takes 3-4 s in JSC. While V8 and SpiderMonkey take 66 ms. > This is not related to encoding, this is for JSC Intl features including
> Intl.DateTimeFormat, NumberFormat etc.
That's why my question was about "anything else".
I guess a microbenchmark is good enough here, although an improvement on an actual live website would be more cool.
I think the goal of this is making Intl APIs performance competitive to the other engines even after adding new features / fixing spec violations / updating the existing implementation. I've checked new Intl APIs and noticed that updating sometimes introduces more ICU round-trip, which can be costly. And I noticed that these cost can be avoided if some of ICU objects are cached because these calculations are typically per-locale, and it seems that the other engines are taking the same path (caching) to mitigate this ICU cost. If we can keep these APIs competitive to the other engines (this means, significantly improve the performance for major cases, while keeping performance same to the other engines for minor cases), then we do not need to bother about updating / fixing the existing issues which can introduce more inevitable ICU round-trips :), that's the goal of this. UDateTimePatternGenerator look up is too expensive. Created attachment 408891 [details]
Patch
Comment on attachment 408891 [details]
Patch
r=me, what an amazing perf boost.
Like you mentioned on Slack, I like the idea of looking into whether an LRU cache or something would provide further benefits in the future.
Committed r267132: <https://trac.webkit.org/changeset/267132> |