Bug 206791 - Move singleton Intl string locales out of JSGlobalObject.
Summary: Move singleton Intl string locales out of JSGlobalObject.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-01-24 18:53 PST by Mark Lam
Modified: 2020-01-28 09:05 PST (History)
14 users (show)

See Also:


Attachments
proposed patch. (25.37 KB, patch)
2020-01-24 19:10 PST, Mark Lam
ysuzuki: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2020-01-24 18:53:55 PST
We're creating an instance of these for each JSGlobalObject when they can be a global singleton since they are always initialized with the same intl data (barring a mid-flight change in intl settings, which we don't support even in the existing code).

Also fix a bug in StringImpl::createStaticStringImpl(): I forgot to set its hash value when I introduced it.  StaticStringImpls require that its hash code be set ahead of time, and cannot be mutated at runtime.
Comment 1 Radar WebKit Bug Importer 2020-01-24 18:54:36 PST
<rdar://problem/58889037>
Comment 2 Andy VanWagoner 2020-01-24 19:08:56 PST
That sounds good to me. I don’t know of any reason not to use a global singleton.
Comment 3 Mark Lam 2020-01-24 19:10:57 PST
Created attachment 388755 [details]
proposed patch.
Comment 4 Mark Lam 2020-01-24 19:13:28 PST
Comment on attachment 388755 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=388755&action=review

> Source/JavaScriptCore/runtime/IntlPluralRules.cpp:124
> -    const HashSet<String> availableLocales = globalObject->intlNumberFormatAvailableLocales();
> +    const HashSet<String>& availableLocales = intlNumberFormatAvailableLocales();

Andy, can you confirm that it is intentional to use intlNumberFormatAvailableLocales() instead of intlPluralRulesAvailableLocales() here?  I noticed that intlPluralRulesAvailableLocales() is called anywhere, which may me suspicious of this code.  Thought you might know better what is correct.
Comment 5 Mark Lam 2020-01-24 19:14:36 PST
(In reply to Mark Lam from comment #4)
> Andy, can you confirm that it is intentional to use
> intlNumberFormatAvailableLocales() instead of
> intlPluralRulesAvailableLocales() here?  I noticed that
> intlPluralRulesAvailableLocales() is called anywhere, which may me
> suspicious of this code.  Thought you might know better what is correct.

typo: I actually meant "intlPluralRulesAvailableLocales() is NOT called anywhere".
Comment 6 Andy VanWagoner 2020-01-24 19:20:28 PST
(In reply to Mark Lam from comment #5)
> (In reply to Mark Lam from comment #4)
> > Andy, can you confirm that it is intentional to use
> > intlNumberFormatAvailableLocales() instead of
> > intlPluralRulesAvailableLocales() here?  I noticed that
> > intlPluralRulesAvailableLocales() is called anywhere, which may me
> > suspicious of this code.  Thought you might know better what is correct.
> 
> typo: I actually meant "intlPluralRulesAvailableLocales() is NOT called
> anywhere".

I’ll double check.
Comment 7 Andy VanWagoner 2020-01-24 20:21:47 PST
The ECMA 402 spec calls for %PluralRules%.[[AvailableLocales]] to be passed into ResolveLocale during InitializePluralRules[0] and used in supportedLocalesOf[1]. I believe this is what intlPluralRulesAvailableLocales was created for.

However, there is no uplrules_getAvailable[2], so intlPluralRulesAvailableLocales was just using uloc_getAvailable (all available locales). It seems a better assumption that number formatting supporting locales would have plural rules support, than all known locales, and in IntlPluralRules::select, sometimes calls unum_formatDouble & unum_formatDouble too, so it seems appropriate to draw the available locales from unum_getAvailable.

IntlPluralRules::initializePluralRules[3] and IntlPluralRulesConstructorFuncSupportedLocalesOf[4] are already just using intlNumberFormatAvailableLocales, and I don't see a reason to duplicate that list into intlPluralRulesAvailableLocales. Let's just drop it entirely.

In JSC %PluralRules%.[[AvailableLocales]] will just be the same as %NumberFormat%.[[AvailableLocales]].

[0] https://tc39.es/ecma402/#sec-initializepluralrules
[1] https://tc39.es/ecma402/#sec-intl.pluralrules.supportedlocalesof
[2] https://unicode-org.github.io/icu-docs/apidoc/released/icu4c/upluralrules_8h.html
[3] https://trac.webkit.org/browser/webkit/trunk/Source/JavaScriptCore/runtime/IntlPluralRules.cpp#L124
[4] https://trac.webkit.org/browser/webkit/trunk/Source/JavaScriptCore/runtime/IntlPluralRulesConstructor.cpp#L120
Comment 8 Andy VanWagoner 2020-01-24 20:27:25 PST
We could have intlPluralRulesAvailableLocales be a reference to intlNumberFormatAvailableLocales, and use that in IntlPluralRules::initializePluralRules and IntlPluralRulesConstructorFuncSupportedLocalesOf.
That would remove the suspect use of intlNumberFormatAvailableLocales.
Comment 9 Yusuke Suzuki 2020-01-24 20:59:14 PST
Comment on attachment 388755 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=388755&action=review

r=me, nice.

> Source/JavaScriptCore/runtime/IntlObject.h:62
> +const HashSet<String>& intlNumberFormatAvailableLocales();

Define intlPluralRulesAvailableLocales.

>> Source/JavaScriptCore/runtime/IntlPluralRules.cpp:124
>> +    const HashSet<String>& availableLocales = intlNumberFormatAvailableLocales();
> 
> Andy, can you confirm that it is intentional to use intlNumberFormatAvailableLocales() instead of intlPluralRulesAvailableLocales() here?  I noticed that intlPluralRulesAvailableLocales() is called anywhere, which may me suspicious of this code.  Thought you might know better what is correct.

Thanks, Andy. Yeah, seems that,

defining intlPluralRulesAvailableLocales, which just calls intlNumberFormatAvailableLocales() internally.

Would be nice.

> Source/JavaScriptCore/runtime/IntlPluralRulesConstructor.cpp:121
> +    const HashSet<String>& availableLocales = intlNumberFormatAvailableLocales();

Yeah, use intlPluralRulesAvailableLocales, which should just call intlNumberFormatAvailableLocales() internally.
Comment 10 Mark Lam 2020-01-25 09:04:53 PST
Thanks for the reviews.  I've applied the requested changes.

Landed in r255120: <http://trac.webkit.org/r255120>.
Comment 11 Mark Lam 2020-01-25 09:09:31 PST
FYI, this patch also contains a fix for a bug in r255112:<http://trac.webkit.org/r255112> which was landed for https://bugs.webkit.org/show_bug.cgi?id=20677.
Comment 12 Darin Adler 2020-01-25 09:46:06 PST
Comment on attachment 388755 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=388755&action=review

> Source/WTF/wtf/text/StringImpl.cpp:287
> +    ASSERT(charactersAreAllASCII<LChar>(lcharCharacters, length));

The <LChar> here isn’t needed.
Comment 13 Darin Adler 2020-01-25 09:48:47 PST
Comment on attachment 388755 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=388755&action=review

> Source/WTF/ChangeLog:12
> +        Fix a bug in StringImpl::createStaticStringImpl(): I forgot to set its hash value
> +        when I introduced it.  StaticStringImpls require that its hash code be set ahead
> +        of time, and cannot be mutated at runtime.  See the comment in the definition of
> +        StaticStringImpl in StringImpl.h.

Do we have a test that shows this is correct now?
Comment 14 Mark Lam 2020-01-25 10:55:33 PST
(In reply to Darin Adler from comment #12)
> > Source/WTF/wtf/text/StringImpl.cpp:287
> > +    ASSERT(charactersAreAllASCII<LChar>(lcharCharacters, length));
> 
> The <LChar> here isn’t needed.

I copied this code over from StringImpl::createFromLiteral().  I'll fix it there too.

(In reply to Darin Adler from comment #13)
> > Source/WTF/ChangeLog:12
> > +        Fix a bug in StringImpl::createStaticStringImpl(): I forgot to set its hash value
> > +        when I introduced it.  StaticStringImpls require that its hash code be set ahead
> > +        of time, and cannot be mutated at runtime.  See the comment in the definition of
> > +        StaticStringImpl in StringImpl.h.
> 
> Do we have a test that shows this is correct now?

This was caught by existing JSC tests.  But I'll add some API tests.

Will do these in https://bugs.webkit.org/show_bug.cgi?id=206802.
Comment 15 Mark Lam 2020-01-28 09:05:56 PST
*** Bug 206869 has been marked as a duplicate of this bug. ***