Created attachment 412666 [details] repro the bug Steps to Repro: simple.com -> Move Money -> “Transfer to or from a bank” The animation when it transitions in is very slow, and every keypress is very slow. The attached reduction shows the problem: the Date constructor is - in some cases - 20-30x slower now than it used to be, and Simple is calling it a /lot/. ggaren says that we're always missing the Date cache, and that adding a Z to the end of the string makes you go fast again (I'll attach a "fixed" version of the repro case too).
Created attachment 412667 [details] fast version with Zs
rdar://problem/70782204
Geoff bisected (I think) this to https://trac.webkit.org/changeset/254038/webkit
Yes, that's right.
I wonder: If you change the test case's strings to specify local time explicitly (rather than relying on there parser's implicit default when not specified), was that always slow before r254038?
(In reply to Tim Horton from comment #0) > ggaren says that we're always missing the Date cache Yes, the date cache is not expected to work here. It only works for one string. We could add a different date cache, but I don’t think the date cache had anything to do with why this was fast before.
Before r254038, a time without a time zone would be parsed as UTC, not as local time. My guess is that *localTimeOffset* is simply slow, and this code would never have called that before r254038 because it wasn’t . To optimize this, one relevant question is why the caching in *localTimeOffset* is not effective.
(In reply to Darin Adler from comment #7) > My guess is that *localTimeOffset* is simply slow, and this code would never > have called that before r254038 because it wasn’t . because it wasn't treating it as a local time.
I think the issue is that finding time offsets has always been slow, and involves calling underlying platform APIs. And our caching is inadequate to mitigate that. And it also seems that websites that don’t need this expensive operation end up asking for it accidentally. It would be easy to fix this slowdown by restoring the old behavior, and much harder to fix it by optimizing local time computations.
If this is much faster in other browser engines, I would love to know how they manage to quickly compute the correct answer.
With only 20 date strings in the whole test, I would expect localTimeOffset() for those strings to be fast, with a nearly 100% cache hit rate. That's the part that surprises me.
(In reply to Geoffrey Garen from comment #11) > With only 20 date strings in the whole test, I would expect > localTimeOffset() for those strings to be fast, with a nearly 100% cache hit > rate. What’s the key for the cache? I am not surprised that there’s a 0% cache hit rate, but maybe I should be.
The key is { milliseconds, WTF::TimeType }. Oh, wait a second. The cache only caches values within the mostly recently accessed month. So, I guess that must be why it's slow: All these time values are in different months.
I think we probably either need to expand the date string cache or expand the locatTimeOffset cache or both.
(In reply to Geoffrey Garen from comment #14) > I think we probably either need to expand the date string cache or expand > the locatTimeOffset cache or both. I've checked and our range caching mechanism fail to find the results. To solve this problem, we need to have much larger cache because this micro benchmark is doing offset lookup for various years, and each year can have slightly different offset information due to DST. Rather than that, we should use ICU's timezone cache directly instead. And that's what V8 and SpiderMonkey are doing (while they have range-cache for non-ICU environment, which is similar to us). One sad thing is ICU's TimeZone is only available in C++... So we need to use C++ ICU APIs for this particular place unfortunately.
> Rather than that, we should use ICU's timezone cache directly instead. And > that's what V8 and SpiderMonkey are doing (while they have range-cache for > non-ICU environment, which is similar to us). Also: v8 seems to use a 32 element cache (holds 32 distinct month ranges) when doing caching explicitly. But yeah, just using ICU's cache would be great!
It’s nice that switching to ICU will solve this problem, but I assume that making that switch will also change other things. Are all those changes improvements? Does ICU respond the way we would like to time zone changes while the process is running, clearing caches as appropriate? Does it use historically accurate DST information, or not? Does it matter that ICU’s time zone database is separate from the Apple platform ones and will be updated at a different time? It would also be straightforward to fix our cache, so we should not treat ICU as our only option. But if it’s a superior option, I am open to it. I’d like to know more about the costs of using ICU’s C++ API. Will there be practical problems with that? Or were we carefully sticking to the C API without a compelling reason?
(In reply to Darin Adler from comment #17) > It’s nice that switching to ICU will solve this problem, but I assume that > making that switch will also change other things. Are all those changes > improvements? > Does ICU respond the way we would like to time zone changes > while the process is running, clearing caches as appropriate? ICU does not respond to that, but ICU offers the way to clear the cache. V8 and SpiderMonkey do different things: SpiderMonkey clears the cache when new Realm is created IIRC. V8 offers the way to clear, but it doesn't clear ICU implicitly. Chrome renderer process clears it by calling it when timezone is changed. Current way of handling timezone cache in JSC is a bit silly: every time we first enter the JSC VM scope. > Does it use > historically accurate DST information, or not? Does it matter that ICU’s > time zone database is separate from the Apple platform ones and will be > updated at a different time? I think rest of Apple platforms uses ICU since CoreFoundation etc. uses it and we need to handle i18n in platform. Possibly, switching to ICU can be improvement in terms of consistency with the platform. > > It would also be straightforward to fix our cache, so we should not treat > ICU as our only option. But if it’s a superior option, I am open to it. I think ICU cache is better in terms of efficiency compared to the blind cache over the localtime_r. ICU can have all timezone data and rules including DST. So ICU can cache timezone data efficiently even if the Date is not within the same year. ICU should know that the DST rule of the timezone in detail, and based on that, it can decide offset efficiently: e.g. ICU can know whether this month is never in DST etc. > > I’d like to know more about the costs of using ICU’s C++ API. Will there be > practical problems with that? Or were we carefully sticking to the C API > without a compelling reason?
Created attachment 412708 [details] Patch
Comment on attachment 412708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412708&action=review Neat! Very red, and I'll let other folks do the real reviewing, but thank you for fixing! > Source/JavaScriptCore/Sources.txt:867 > +// Confine U_SHOW_CPLUSPLUS_API's effect in this file. I think "to" instead of "in". > JSTests/microbenchmarks/local-date-constructor.js:1 > +function doTheThing() { Heh, now I wish I had named this something better.
OK, we need to update our test results. Our results tested in our test suites were wrong. Our test is using `new Date(1111, 1)` etc. And it assumes that the timezone is the same America/Los_Angeles. But this is wrong strictly speaking. America/Los_Angeles timezone is effective only after 1991. Before that, we must handle it UTC-0752, which is based on Los_Angeles' longitude (118.2347W).
Comment on attachment 412708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412708&action=review >> Source/JavaScriptCore/Sources.txt:867 >> +// Confine U_SHOW_CPLUSPLUS_API's effect in this file. > > I think "to" instead of "in". Fixed. >> JSTests/microbenchmarks/local-date-constructor.js:1 >> +function doTheThing() { > > Heh, now I wish I had named this something better. Renamed it to "test" :)
Created attachment 412726 [details] Patch
Created attachment 412883 [details] Patch
Created attachment 412884 [details] Patch
Created attachment 412886 [details] Patch
Created attachment 412889 [details] Patch
Created attachment 412949 [details] Patch
Comment on attachment 412949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412949&action=review Seems like the refactoring to make everything instance functions could be done separately/first if we want to keep the big behavior changing patch smalle. > Source/JavaScriptCore/Sources.txt:868 > +// Confine U_SHOW_CPLUSPLUS_API's effect to this file. > +runtime/JSDateMath.cpp @no-unify A comment like this is more likely to stay with the line below if it’s at the end of the line. Since this is a file we keep sorted that doesn’t have comments interspersed, I worry that this comment won’t be obviously bound to exactly one line after it. But also, given the #undef tricks we do, and the fact that these are not used in other files, is this really needed in practice? Does the build break if we don’t make this change? > Source/JavaScriptCore/runtime/JSDateMath.cpp:108 > + constexpr bool isLocalTime = false; Not personally a huge fan of this way of documenting the meaning of an argument. Normally I prefer our enum technique instead. > Source/JavaScriptCore/runtime/JSDateMath.cpp:189 > +Ref<DateInstanceData> DateCache::getOrCreateCachedDateInstanceData(double milli) Not so fond of "miill" as the name for "date/time in milliseconds, relative to the 1970 epoch". > Source/JavaScriptCore/runtime/JSDateMath.h:58 > +// We do not expose icu::TimeZone in this header file. And we cannot use icu::TimeZone forward declaration > +// because icu namespace can be an alias to icu$verNum namespace. > +class OpaqueICUTimeZone; > +struct OpaqueICUTimeZoneDeleter { > + JS_EXPORT_PRIVATE void operator()(OpaqueICUTimeZone*); > +}; I don’t think the forward declaration of OpaqueICUTimeZone needs this comment. The comment is for the deleter. Lets put the forward declaration above init the normal forward declaration paragraph. And put the deleter in its own paragraph. Doesn’t need to be grouped with the forward declaration. > Source/JavaScriptCore/runtime/JSDateMath.h:69 > + OpaqueICUTimeZone* ensureTimeZoneCache() Not sure why we need verbs like "ensure". This function returns the cache. And yes, allocates it if needed. Such a function could just be named "timeZoneCache", I think. > Source/JavaScriptCore/runtime/JSDateMath.h:76 > + Ref<DateInstanceData> getOrCreateCachedDateInstanceData(double); Not sure why we need verbs like "get or create" here. This function returns cached instance data. And yes, allocates it if needed. Such a function could just be named "cachedDateInstanceData", I think. I don’t think the meaning of the argument is obvious. The fact that the double is the date, and what kind of double it is (in milliseconds, in seconds, what base?). The other functions have "milliseconds" in their names. Kind of wish we could use our classes that disambiguate what the double is used for. > Source/JavaScriptCore/runtime/JSDateMath.h:78 > + void msToGregorianDateTime(double, WTF::TimeType outputTimeType, GregorianDateTime&); Some day we could use a return value here instead of an out argument. And give this a better name perhaps? > Source/JavaScriptCore/runtime/JSDateMath.h:79 > + double gregorianDateTimeToMS(const GregorianDateTime&, double, WTF::TimeType inputTimeType); Really unclear what the double argument is. > Source/JavaScriptCore/runtime/JSDateMath.h:80 > + double parseDate(JSGlobalObject*, VM&, const WTF::String&); Can this take a StringView instead of a const String& some day?
Comment on attachment 412949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412949&action=review > LayoutTests/js/script-tests/date-constructor.js:10 > +object.valueOf = function() { return 1995; } why the change of this test?
Comment on attachment 412949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412949&action=review Unfortunately, I think including DateCache refactoring is a bit important to include everything into JSDateMath.cpp and not leaking icu C++ API usage outside of JSDateMath.cpp. For example, currently, we are having VM::resetDateCache and it is clearing things in VM.cpp. We do not want to touch DateCache things in non JSDateMath.cpp and we would like to confine all operations into JSDateMath.cpp. That's why this patch involves these changes. >> Source/JavaScriptCore/Sources.txt:868 >> +runtime/JSDateMath.cpp @no-unify > > A comment like this is more likely to stay with the line below if it’s at the end of the line. Since this is a file we keep sorted that doesn’t have comments interspersed, I worry that this comment won’t be obviously bound to exactly one line after it. > > But also, given the #undef tricks we do, and the fact that these are not used in other files, is this really needed in practice? Does the build break if we don’t make this change? I don't think removing @no-unify will not break JSC right now. But one thing I care is that, if some cpp file includes header and it includes icu C++ headers without U_SHOW_CPLUSPLUS_API, then it is possible that we will see the problem if such a cpp file is bundled together with this file. I think confine the risk into this one file. The risk is including accidentally using C++ APIs randomly. >> Source/JavaScriptCore/runtime/JSDateMath.cpp:108 >> + constexpr bool isLocalTime = false; > > Not personally a huge fan of this way of documenting the meaning of an argument. Normally I prefer our enum technique instead. This icu::TimeZone::getOffset is ICU API :) >> Source/JavaScriptCore/runtime/JSDateMath.cpp:189 >> +Ref<DateInstanceData> DateCache::getOrCreateCachedDateInstanceData(double milli) > > Not so fond of "miill" as the name for "date/time in milliseconds, relative to the 1970 epoch". I'll change it to milliSecondsFromEpoch. This is because of existing functions in DateInstanceCache etc. >> Source/JavaScriptCore/runtime/JSDateMath.h:58 >> +}; > > I don’t think the forward declaration of OpaqueICUTimeZone needs this comment. The comment is for the deleter. Lets put the forward declaration above init the normal forward declaration paragraph. And put the deleter in its own paragraph. Doesn’t need to be grouped with the forward declaration. OK, changed. >> Source/JavaScriptCore/runtime/JSDateMath.h:69 >> + OpaqueICUTimeZone* ensureTimeZoneCache() > > Not sure why we need verbs like "ensure". This function returns the cache. And yes, allocates it if needed. Such a function could just be named "timeZoneCache", I think. OK, changed :) >> Source/JavaScriptCore/runtime/JSDateMath.h:76 >> + Ref<DateInstanceData> getOrCreateCachedDateInstanceData(double); > > Not sure why we need verbs like "get or create" here. This function returns cached instance data. And yes, allocates it if needed. Such a function could just be named "cachedDateInstanceData", I think. > > I don’t think the meaning of the argument is obvious. The fact that the double is the date, and what kind of double it is (in milliseconds, in seconds, what base?). The other functions have "milliseconds" in their names. Kind of wish we could use our classes that disambiguate what the double is used for. OK, changed this name to cachedDateInstanceData and put millisecondsFromEpoch variable. >> Source/JavaScriptCore/runtime/JSDateMath.h:78 >> + void msToGregorianDateTime(double, WTF::TimeType outputTimeType, GregorianDateTime&); > > Some day we could use a return value here instead of an out argument. And give this a better name perhaps? Yeah, we should reconsider naming of this, in a separate patch. This code is super old one. I'll also add milliSecondsFromEpoch variable name to this double. >> Source/JavaScriptCore/runtime/JSDateMath.h:79 >> + double gregorianDateTimeToMS(const GregorianDateTime&, double, WTF::TimeType inputTimeType); > > Really unclear what the double argument is. This code is existing one. But I'll add `milliSecondsFromEpoch` to this variable. >> Source/JavaScriptCore/runtime/JSDateMath.h:80 >> + double parseDate(JSGlobalObject*, VM&, const WTF::String&); > > Can this take a StringView instead of a const String& some day? Interestingly, using WTF::String& is more efficient than StringView here. The reason is that we could cache this input string in m_cachedDateString. And this is String. If we get this as `const WTF::String&`, then copying this to m_cachedDateString is ref(). But if we take this as StringView, we need to copy the content to m_cachedDateString.
Created attachment 412972 [details] Patch
Comment on attachment 412949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412949&action=review >> LayoutTests/js/script-tests/date-constructor.js:10 >> +object.valueOf = function() { return 1995; } > > why the change of this test? The reason is described in ChangeLog. "The time before America/Los_Angeles timezone is effective should be handled as UTC-0752. Use 1995 to test the intent correctly." Between 1111 and 1995, timezones are different. One is UTC-0752, and one is America/Los_Angeles. This difference is tested in different tests. And in this test, we would like to avoid this difference because it is out of intent of this test.
Comment on attachment 412949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412949&action=review >>> Source/JavaScriptCore/Sources.txt:868 >>> +runtime/JSDateMath.cpp @no-unify >> >> A comment like this is more likely to stay with the line below if it’s at the end of the line. Since this is a file we keep sorted that doesn’t have comments interspersed, I worry that this comment won’t be obviously bound to exactly one line after it. >> >> But also, given the #undef tricks we do, and the fact that these are not used in other files, is this really needed in practice? Does the build break if we don’t make this change? > > I don't think removing @no-unify will not break JSC right now. But one thing I care is that, if some cpp file includes header and it includes icu C++ headers without U_SHOW_CPLUSPLUS_API, then it is possible that we will see the problem if such a cpp file is bundled together with this file. > I think confine the risk into this one file. The risk is including accidentally using C++ APIs randomly. OK. What about my first comment, that it should be an "end of line" comment, not a "line before" comment? >>> Source/JavaScriptCore/runtime/JSDateMath.h:78 >>> + void msToGregorianDateTime(double, WTF::TimeType outputTimeType, GregorianDateTime&); >> >> Some day we could use a return value here instead of an out argument. And give this a better name perhaps? > > Yeah, we should reconsider naming of this, in a separate patch. This code is super old one. I'll also add milliSecondsFromEpoch variable name to this double. Great. (Please don’t capitalize the "S" in "milliseconds".) > Source/JavaScriptCore/runtime/VMEntryScope.cpp:46 > + // FIXME: We should clear it only when we know that timezone has been changed. "that timezone" -> "the timezone".
Comment on attachment 412949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412949&action=review >>>> Source/JavaScriptCore/Sources.txt:868 >>>> +runtime/JSDateMath.cpp @no-unify >>> >>> A comment like this is more likely to stay with the line below if it’s at the end of the line. Since this is a file we keep sorted that doesn’t have comments interspersed, I worry that this comment won’t be obviously bound to exactly one line after it. >>> >>> But also, given the #undef tricks we do, and the fact that these are not used in other files, is this really needed in practice? Does the build break if we don’t make this change? >> >> I don't think removing @no-unify will not break JSC right now. But one thing I care is that, if some cpp file includes header and it includes icu C++ headers without U_SHOW_CPLUSPLUS_API, then it is possible that we will see the problem if such a cpp file is bundled together with this file. >> I think confine the risk into this one file. The risk is including accidentally using C++ APIs randomly. > > OK. What about my first comment, that it should be an "end of line" comment, not a "line before" comment? Ah, yeah, that sounds good. I've changed.
Comment on attachment 412949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412949&action=review >>>> Source/JavaScriptCore/runtime/JSDateMath.h:78 >>>> + void msToGregorianDateTime(double, WTF::TimeType outputTimeType, GregorianDateTime&); >>> >>> Some day we could use a return value here instead of an out argument. And give this a better name perhaps? >> >> Yeah, we should reconsider naming of this, in a separate patch. This code is super old one. I'll also add milliSecondsFromEpoch variable name to this double. > > Great. (Please don’t capitalize the "S" in "milliseconds".) Fixed. >>> Source/JavaScriptCore/runtime/JSDateMath.h:79 >>> + double gregorianDateTimeToMS(const GregorianDateTime&, double, WTF::TimeType inputTimeType); >> >> Really unclear what the double argument is. > > This code is existing one. But I'll add `milliSecondsFromEpoch` to this variable. Interestingly, this double is not millisecondsFromEpoch. It is milliseconds unit of GregorianDateTime. So I named it as `milliseconds` while msToGregorianDateTime's double is `millisecondsFromEpoch`. >> Source/JavaScriptCore/runtime/VMEntryScope.cpp:46 >> + // FIXME: We should clear it only when we know that timezone has been changed. > > "that timezone" -> "the timezone". Fixed.
Created attachment 412976 [details] Patch
Comment on attachment 412976 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412976&action=review > Source/JavaScriptCore/runtime/JSDateMath.cpp:119 > + return LocalTimeOffset { false, 0 }; > + return LocalTimeOffset { !!dstOffset, rawOffset + dstOffset }; We don’t actually have to write LocalTimeOffset here, do we? > JSTests/ChangeLog:20 > + * ChakraCore.yaml: > + * ChakraCore/test/Date/DateCtr.baseline-jsc: Added. The time before America/Los_Angeles timezone is effective should be handled as UTC-0752. > + * complex.yaml: > + * complex/timezone-offset-before-america-los-angeles-is-defined.js: Added for UTC-0752. > + (shouldBe): > + * microbenchmarks/local-date-constructor.js: Added. > + (test): > + * mozilla/ecma/Date/15.9.5.16.js: > + * mozilla/ecma/Date/15.9.5.18.js: > + * mozilla/ecma/Date/15.9.5.22-1.js: > + * mozilla/ecma/Date/15.9.5.22-2.js: > + * mozilla/ecma/Date/15.9.5.35-1.js: > + (getTestCases): I wish this explained the test changes better. Could get rid of the silly quoting of the function names, and add comments explaining the reasons these are good changes to make. > JSTests/mozilla/ecma/Date/15.9.5.16.js:-58 > - addTestCase( TIME_YEAR_0 ); The comments don’t explain this. I think I understand it, but it would have been good to explain it in the change log. > JSTests/mozilla/ecma/Date/15.9.5.18.js:-59 > - addTestCase( TIME_YEAR_0 ); Ditto. > JSTests/mozilla/ecma/Date/15.9.5.22-1.js:-61 > - addTestCase( TIME_YEAR_0 ); Ditto. > JSTests/mozilla/ecma/Date/15.9.5.22-2.js:60 > - addTestCase( TIME_YEAR_0 ); > /* > + addTestCase( TIME_YEAR_0 ); Ditto.
Comment on attachment 412976 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412976&action=review Thanks! >> Source/JavaScriptCore/runtime/JSDateMath.cpp:119 >> + return LocalTimeOffset { !!dstOffset, rawOffset + dstOffset }; > > We don’t actually have to write LocalTimeOffset here, do we? Yes, we do not need to have LocalTimeOffset here. Removed. >> JSTests/ChangeLog:20 >> + (getTestCases): > > I wish this explained the test changes better. Could get rid of the silly quoting of the function names, and add comments explaining the reasons these are good changes to make. Added explanation here. >> JSTests/mozilla/ecma/Date/15.9.5.16.js:-58 >> - addTestCase( TIME_YEAR_0 ); > > The comments don’t explain this. I think I understand it, but it would have been good to explain it in the change log. Added explanation in ChangeLog. >> JSTests/mozilla/ecma/Date/15.9.5.18.js:-59 >> - addTestCase( TIME_YEAR_0 ); > > Ditto. Added explanation in ChangeLog. >> JSTests/mozilla/ecma/Date/15.9.5.22-1.js:-61 >> - addTestCase( TIME_YEAR_0 ); > > Ditto. Added explanation in ChangeLog. >> JSTests/mozilla/ecma/Date/15.9.5.22-2.js:60 >> + addTestCase( TIME_YEAR_0 ); > > Ditto. Added explanation in ChangeLog.
Committed r269320: <https://trac.webkit.org/changeset/269320>
Re-opened since this is blocked by bug 219915
The reverted patch is once integrated since we workaround ICU missing API issue differently.
(In reply to Yusuke Suzuki from comment #42) > The reverted patch is once integrated since we workaround ICU missing API > issue differently. I’d like to know more. Where should I look to find out?
(In reply to Darin Adler from comment #43) > (In reply to Yusuke Suzuki from comment #42) > > The reverted patch is once integrated since we workaround ICU missing API > > issue differently. > > I’d like to know more. Where should I look to find out? Internal discussion, I'll send you a link :)
*** Bug 196764 has been marked as a duplicate of this bug. ***