Only clear ICU cache when time zone is changed. Otherwise, VMEntryScope keeps invalidating the ICU cache and we keep going to the file system to figure out the current host time zone: Sample Count, Samples %, Normalized CPU %, Symbol 2, 0.0%, 0.0%, JSC::DateCache::defaultTimeZone() (in JavaScriptCore) 2, 0.0%, 0.0%, JSC::DateCache::timeZoneCacheSlow() (in JavaScriptCore) 2, 0.0%, 0.0%, ucal_getHostTimeZone (in libicucore.A.dylib) 2, 0.0%, 0.0%, icu::TimeZone::detectHostTimeZone() (in libicucore.A.dylib) 1, 0.0%, 0.0%, readlink (in libsystem_kernel.dylib) 1, 0.0%, 0.0%, unix_syscall64 (in kernel.development) 1, 0.0%, 0.0%, readlink (in kernel.development)
Created attachment 452774 [details] Patch
Created attachment 452873 [details] Patch
Comment on attachment 452873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=452873&action=review > Source/JavaScriptCore/runtime/JSDateMath.cpp:358 > + static_cast<DateCache*>(observer)->reset(); I think this needs some care: in non-WebKit JavaScriptCore.framework, JSC VM can be moved from one thread to the other. So let's consider the case, 1. Initializing JSC VM on the main thread 2. It registers notification 3. Then moving this VM to a different thread 4. Using this VM in the different thread 5. Now notification comes to the main thread 6. We reset the cache from the main thread while the different thread is using VM, which can be in the middle of using this cache.
(In reply to Yusuke Suzuki from comment #3) > Comment on attachment 452873 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=452873&action=review > > > Source/JavaScriptCore/runtime/JSDateMath.cpp:358 > > + static_cast<DateCache*>(observer)->reset(); > > I think this needs some care: in non-WebKit JavaScriptCore.framework, JSC VM > can be moved from one thread to the other. So let's consider the case, > > 1. Initializing JSC VM on the main thread > 2. It registers notification > 3. Then moving this VM to a different thread > 4. Using this VM in the different thread > 5. Now notification comes to the main thread > 6. We reset the cache from the main thread while the different thread is > using VM, which can be in the middle of using this cache. Ouch, I didn't realize a VM would be moved to a different thread. If so, then it makes things more complicated indeed..
Created attachment 453018 [details] Patch
Comment on attachment 453018 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453018&action=review > Source/JavaScriptCore/runtime/JSDateMath.h:135 > +inline OpaqueICUTimeZone* DateCache::timeZoneCache() > +{ > + auto& caches = ensureCaches(); > + if (!caches.timeZoneCache) > + caches.timeZoneCacheSlow(); > + return caches.timeZoneCache.get(); > +} > + > +inline auto DateCache::ensureCaches() -> Caches& > +{ > + if (!m_caches || !m_caches->isUpToDate()) > + m_caches.emplace(); > + return *m_caches; > +} Let's reset DateCache only when entering VMEntryScope. In the current implementation, DateCache can be reset and cleared anytime we use a cache. This means that, when running a code, let date1 = new Date(); let date2 = new Date(); in the above sequence, suddenly, date1 and date2 get different timezone, which is surprising. So, instead, let's ensure the same timezone while running one VMEntryScope, and clear if necessary when entering next VMEntryScope.
Comment on attachment 453018 [details] Patch Can do :)
Created attachment 453058 [details] Patch
Comment on attachment 453058 [details] Patch r=me
Comment on attachment 453058 [details] Patch Thanks for reviewing.
Comment on attachment 453058 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453058&action=review > Source/JavaScriptCore/runtime/JSDateMath.cpp:370 > +#if PLATFORM(COCOA) > +static void timeZoneChangeNotification(CFNotificationCenterRef, void*, CFStringRef, const void*, CFDictionaryRef) > +{ > + ASSERT(isMainThread()); > + ++lastTimeZoneID; > +} > +#endif > + > // To confine icu::TimeZone destructor invocation in this file. > -DateCache::DateCache() = default; > +DateCache::DateCache() > +{ > +#if PLATFORM(COCOA) > + static std::once_flag onceKey; > + std::call_once(onceKey, [&] { > + CFNotificationCenterAddObserver(CFNotificationCenterGetLocalCenter(), nullptr, timeZoneChangeNotification, kCFTimeZoneSystemTimeZoneDidChangeNotification, nullptr, CFNotificationSuspensionBehaviorDeliverImmediately); > + }); Will it always notified into the main thread even if we are calling this from non main thread? (in JavaScriptCore.framework use case, it is possible that VM's first launch is on the non main thread.)
(In reply to Yusuke Suzuki from comment #11) > Comment on attachment 453058 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=453058&action=review > > > Source/JavaScriptCore/runtime/JSDateMath.cpp:370 > > +#if PLATFORM(COCOA) > > +static void timeZoneChangeNotification(CFNotificationCenterRef, void*, CFStringRef, const void*, CFDictionaryRef) > > +{ > > + ASSERT(isMainThread()); > > + ++lastTimeZoneID; > > +} > > +#endif > > + > > // To confine icu::TimeZone destructor invocation in this file. > > -DateCache::DateCache() = default; > > +DateCache::DateCache() > > +{ > > +#if PLATFORM(COCOA) > > + static std::once_flag onceKey; > > + std::call_once(onceKey, [&] { > > + CFNotificationCenterAddObserver(CFNotificationCenterGetLocalCenter(), nullptr, timeZoneChangeNotification, kCFTimeZoneSystemTimeZoneDidChangeNotification, nullptr, CFNotificationSuspensionBehaviorDeliverImmediately); > > + }); > > Will it always notified into the main thread even if we are calling this > from non main thread? (in JavaScriptCore.framework use case, it is possible > that VM's first launch is on the non main thread.) Per my understanding of the documentation (https://developer.apple.com/documentation/corefoundation/1543316-cfnotificationcenteraddobserver?language=objc), yes, it would always get called on the main thread.
(In reply to Chris Dumez from comment #12) > (In reply to Yusuke Suzuki from comment #11) > > Comment on attachment 453058 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=453058&action=review > > > > > Source/JavaScriptCore/runtime/JSDateMath.cpp:370 > > > +#if PLATFORM(COCOA) > > > +static void timeZoneChangeNotification(CFNotificationCenterRef, void*, CFStringRef, const void*, CFDictionaryRef) > > > +{ > > > + ASSERT(isMainThread()); > > > + ++lastTimeZoneID; > > > +} > > > +#endif > > > + > > > // To confine icu::TimeZone destructor invocation in this file. > > > -DateCache::DateCache() = default; > > > +DateCache::DateCache() > > > +{ > > > +#if PLATFORM(COCOA) > > > + static std::once_flag onceKey; > > > + std::call_once(onceKey, [&] { > > > + CFNotificationCenterAddObserver(CFNotificationCenterGetLocalCenter(), nullptr, timeZoneChangeNotification, kCFTimeZoneSystemTimeZoneDidChangeNotification, nullptr, CFNotificationSuspensionBehaviorDeliverImmediately); > > > + }); > > > > Will it always notified into the main thread even if we are calling this > > from non main thread? (in JavaScriptCore.framework use case, it is possible > > that VM's first launch is on the non main thread.) > > Per my understanding of the documentation > (https://developer.apple.com/documentation/corefoundation/1543316- > cfnotificationcenteraddobserver?language=objc), yes, it would always get > called on the main thread. That said, we could also drop the main thread assertion. In this latest patch iteration, it doesn't really matter what thread the callback is called on..
(In reply to Chris Dumez from comment #13) > (In reply to Chris Dumez from comment #12) > > (In reply to Yusuke Suzuki from comment #11) > > > Comment on attachment 453058 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=453058&action=review > > > > > > > Source/JavaScriptCore/runtime/JSDateMath.cpp:370 > > > > +#if PLATFORM(COCOA) > > > > +static void timeZoneChangeNotification(CFNotificationCenterRef, void*, CFStringRef, const void*, CFDictionaryRef) > > > > +{ > > > > + ASSERT(isMainThread()); > > > > + ++lastTimeZoneID; > > > > +} > > > > +#endif > > > > + > > > > // To confine icu::TimeZone destructor invocation in this file. > > > > -DateCache::DateCache() = default; > > > > +DateCache::DateCache() > > > > +{ > > > > +#if PLATFORM(COCOA) > > > > + static std::once_flag onceKey; > > > > + std::call_once(onceKey, [&] { > > > > + CFNotificationCenterAddObserver(CFNotificationCenterGetLocalCenter(), nullptr, timeZoneChangeNotification, kCFTimeZoneSystemTimeZoneDidChangeNotification, nullptr, CFNotificationSuspensionBehaviorDeliverImmediately); > > > > + }); > > > > > > Will it always notified into the main thread even if we are calling this > > > from non main thread? (in JavaScriptCore.framework use case, it is possible > > > that VM's first launch is on the non main thread.) > > > > Per my understanding of the documentation > > (https://developer.apple.com/documentation/corefoundation/1543316- > > cfnotificationcenteraddobserver?language=objc), yes, it would always get > > called on the main thread. > > That said, we could also drop the main thread assertion. In this latest > patch iteration, it doesn't really matter what thread the callback is called > on.. Perfect! Thank you.
Committed r290449 (247751@main): <https://commits.webkit.org/247751@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 453058 [details].
<rdar://problem/89432384>
*** Bug 218365 has been marked as a duplicate of this bug. ***