Bug 236996 – [Cocoa] Only clear ICU cache when time zone is changed

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Chris Dumez]

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)

[Chris Dumez]

Created attachment 452774 [details]
Patch

[Chris Dumez]

Created attachment 452873 [details]
Patch

[Yusuke Suzuki]

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.

[Chris Dumez]

(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..

[Chris Dumez]

Created attachment 453018 [details]
Patch

[Yusuke Suzuki]

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.

[Chris Dumez]

Comment on attachment 453018 [details]
Patch

Can do :)

[Chris Dumez]

Created attachment 453058 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 453058 [details]
Patch

r=me

[Chris Dumez]

Comment on attachment 453058 [details]
Patch

Thanks for reviewing.

[Yusuke Suzuki]

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.)

[Chris Dumez]

(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.

[Chris Dumez]

(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..

[Yusuke Suzuki]

(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.

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/89432384>

[Yusuke Suzuki]

*** Bug 218365 has been marked as a duplicate of this bug. ***