1342647 - CurrencyDigits code in Intl.NumberFormat implementation could be cleaned up

Status: NEW

(Core :: JavaScript: Internationalization API, defect, P5)

Description

[Daniel Ehrenberg]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.87 Safari/537.36

Steps to reproduce:

In Intl.NumberFormat, I was wondering how SpiderMonkey gets the default number of digits to display for a currency.


Actual results:

It turns out CurrencyDigits is periodically scraped from somewhere into a JavaScript file.


Expected results:

We should be calling into ucurr_getDefaultFractionDigits , an ICU API which solves exactly this problem. I have a patch in progress to do this.

Comment 1

[Daniel Ehrenberg]

Attachment: Bug 1342647 - Remove redundant IntlCurrency system and use ICU,

Intl.NumberFormat refers to a CurrencyDigits() algorithm which
finds the default number of digits to format a currency in.
SpiderMonkey implemented this by a table off to the side, based
on information scraped from a website. However, ICU has this
information already, accessible by the
ucurr_getDefaultFractionDigits function. This patch calls out
to that function.

Review commit: https://reviewboard.mozilla.org/r/115488/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/115488/

Comment 2

[Daniel Ehrenberg]

Comment on attachment 8841169 [details]
Bug 1342647 - Remove redundant IntlCurrency system and use ICU,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115488/diff/1-2/

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 8841169 [details]
Bug 1342647 - Remove redundant IntlCurrency system and use ICU,

https://reviewboard.mozilla.org/r/115488/#review117224

::: js/src/builtin/Intl.cpp:1385
(Diff revision 2)
> +    if (!stableChars.initTwoByte(cx, str))
> +        return false;
> +    mozilla::Range<const char16_t> chars = stableChars.twoByteRange();
> +    // This might not be null-terminated, but ICU only ever looks
> +    // at the first three UChars, since an ISO currency code always has a
> +    // length of three

e_e

Hmm, this dependence is a bit squirrely.  But also, if this is really true, it suggests we're being super-inefficient here.  Stable-stringing and other gunk is a ton of overhead for just three chars -- might as well just do it manually into a stacked array.  So how about this instead:

    char16_t chars[4];
    {
        JSLinearString* currency = args[0].toString()->ensureLinear(cx);
        if (!currency)
            return false;

        MOZ_ASSERT(currency->length() == 3);
        for (size_t i = 0; i < 3; i++)
            chars[i] = currency->latin1OrTwoByteChar(i);
        chars[3] = '\0';
    }

so that we're not relying on this weird not-actually-null-terminated thing.

Comment 4

[Daniel Ehrenberg]

Attachment: Use simpler method of conversion and always get a null-terminated string

Review commit: https://reviewboard.mozilla.org/r/115974/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/115974/

Comment 5

[Daniel Ehrenberg]

Comment on attachment 8841169 [details]
Bug 1342647 - Remove redundant IntlCurrency system and use ICU,

https://reviewboard.mozilla.org/r/115488/#review117224

> e_e
> 
> Hmm, this dependence is a bit squirrely.  But also, if this is really true, it suggests we're being super-inefficient here.  Stable-stringing and other gunk is a ton of overhead for just three chars -- might as well just do it manually into a stacked array.  So how about this instead:
> 
>     char16_t chars[4];
>     {
>         JSLinearString* currency = args[0].toString()->ensureLinear(cx);
>         if (!currency)
>             return false;
> 
>         MOZ_ASSERT(currency->length() == 3);
>         for (size_t i = 0; i < 3; i++)
>             chars[i] = currency->latin1OrTwoByteChar(i);
>         chars[3] = '\0';
>     }
> 
> so that we're not relying on this weird not-actually-null-terminated thing.

Good point; I was uncomfortable with the phrasing too, just not sure how to do it right in SpiderMonkey. I changed the implementation to roughly what you wrote. What do you think?

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 8841920 [details]
Use simpler method of conversion and always get a null-terminated string

https://reviewboard.mozilla.org/r/115974/#review117472

::: js/src/builtin/Intl.cpp:1378
(Diff revision 1)
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
>      MOZ_ASSERT(args.length() == 1);
>      MOZ_ASSERT(args[0].isString());
>  
> -    RootedString str(cx, args[0].toString());
> +    UChar chars[4];

It's somewhat unusual for us to use UChar other than as the result of a BlahToUChar, but yeah, I guess it makes sense to use it directly here, without the char16_t go-between.  Can't wait for new-enough ICU where we can drop the UChar pretense entirely.  (Well, I guess maybe Windows holds us back there.  Never mind.  Pfui.)

Comment 7

[Daniel Ehrenberg]

Comment on attachment 8841920 [details]
Use simpler method of conversion and always get a null-terminated string

https://reviewboard.mozilla.org/r/115974/#review117472

> It's somewhat unusual for us to use UChar other than as the result of a BlahToUChar, but yeah, I guess it makes sense to use it directly here, without the char16_t go-between.  Can't wait for new-enough ICU where we can drop the UChar pretense entirely.  (Well, I guess maybe Windows holds us back there.  Never mind.  Pfui.)

Would you prefer an explicit static_cast<UChar>()?

Comment 8

[Jeff Walden [:Waldo]]

No need for static_cast<>, if I'd wanted it I would have said so explicitly and probably r-'d the patch.  :-)

Mozreview is a dumb and doesn't want to let me do pushbutton try-pushing (wonder if my LDAP bits are busted somehow as far as Mozreview is concerned -- I don't use it myself, so it could well have been so broken for years), so I had to export the raw patch and push it manually.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35c1b6b0365d2e06308557d1b22e947bcb4a0c71

That's based on some random inbound rev, so be aware that a failure in the push is not *necessarily* an indication that this patch is bad.  A quick skim of the first bit of |hg in| suggests it should be good -- no backouts of stuff that hadn't been landed since my pull -- but that's a fallible heuristic.

Comment 9

[Daniel Ehrenberg]

Sorry, this patch is on hold since it actually introduces a spec violation, which is why it fails the automated tests. IMO the new behavior is actually better than the old, but the standards discussion needs to come first. See https://github.com/tc39/ecma402/issues/134 for more details.

Comment 10

[Jeff Walden [:Waldo]]

<3 tests.  No need to have to explicitly consider the wider world of what might be known to be mistaken if it'll get caught in automation!

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.