Bug 213454 - [JSC] Cache UDateTimePatternGenerator
Summary: [JSC] Cache UDateTimePatternGenerator
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks: 213425
  Show dependency treegraph
 
Reported: 2020-06-22 01:40 PDT by Yusuke Suzuki
Modified: 2020-09-15 21:24 PDT (History)
9 users (show)

See Also:


Attachments
Patch (19.52 KB, patch)
2020-09-15 20:02 PDT, Yusuke Suzuki
ross.kirsling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2020-06-22 01:40:54 PDT
A lot of ICU objects are opened by the specified locale, but in the typical application, this locale can be one.
We should cache some of ICU objects that is repeatedly used, and clear it if it is not used so much.
Comment 1 Alexey Proskuryakov 2020-06-22 08:32:43 PDT
We have a cache for encoding, does anything else show on profiles?
Comment 2 Yusuke Suzuki 2020-06-22 08:34:22 PDT
(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.
Comment 3 Yusuke Suzuki 2020-06-22 09:00:04 PDT
(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.
Comment 4 Alexey Proskuryakov 2020-06-22 09:49:01 PDT
> 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.
Comment 5 Yusuke Suzuki 2020-06-22 17:05:49 PDT
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.
Comment 6 Yusuke Suzuki 2020-09-15 17:44:47 PDT
UDateTimePatternGenerator look up is too expensive.
Comment 7 Yusuke Suzuki 2020-09-15 20:02:45 PDT
Created attachment 408891 [details]
Patch
Comment 8 Ross Kirsling 2020-09-15 20:36:35 PDT
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.
Comment 9 Yusuke Suzuki 2020-09-15 21:23:57 PDT
Committed r267132: <https://trac.webkit.org/changeset/267132>
Comment 10 Radar WebKit Bug Importer 2020-09-15 21:24:15 PDT
<rdar://problem/68962794>