1398185 - Use null prototype for default options in Intl.Collator and Intl.NumberFormat

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, enhancement, P3)

Description

[André Bargull [:anba]]

Implement changes from https://github.com/tc39/ecma402/pull/170.

Then we can close bug 1296233 as Invalid.

Comment 1

[André Bargull [:anba]]

PR was just merged, taking.

Comment 2

[André Bargull [:anba]]

Attachment: bug1398185-part1-use-null-proto.patch

Updates Intl.Collator and Intl.NumberFormat to use |ObjectCreate(null)| instead of |ObjectCreate(%ObjectPrototype%)| per the spec PR. Also updates Intl.PluralRules, Intl.RelativeTimeFormat, and Intl.getDisplayNames in good faith that the champions for these proposals will apply the same changes. :-)

Comment 3

[André Bargull [:anba]]

Attachment: bug1398185-part2-remove-more-extra-rooting.patch

Oh look, we can remove some more extraneous rooting in Intl code! :-)

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8917865 [details] [diff] [review]
bug1398185-part1-use-null-proto.patch

Review of attachment 8917865 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Intl.js
@@ +1680,5 @@
>      // Steps 4-5.
>      //
>      // If we ever need more speed here at startup, we should try to detect the
> +    // case where |options === undefined| and then directly use the default
> +    // value for each option.  For now, just keep it simple.

I am finding the spec algorithm more and more offensive here, in that it's not styled

    1. If |options| is undefined,
       1. Let usage be "sort".
       1. ...
    1. Else
       1. Let |options| be |ToObject(options)|.
       1. ...

but if I ever get that fed up, I probably should just PR it upstream and then make such change here.  We're good with this now, of course.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8917866 [details] [diff] [review]
bug1398185-part2-remove-more-extra-rooting.patch

Review of attachment 8917866 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not super-happy about removing unnecessary rooting when the relevant variable's scope is tens of lines or more, as a general rule.  It's one thing for a few lines with no |cx| usage in sight, but it's very different when it's a lot more and you just can't clearly see that at a glance.  Let's try to ameliorate that with a few more narrowly-scoped blocks or helper functions to minimize unrooted things' scope.

::: js/src/builtin/Intl.cpp
@@ +3677,5 @@
>          return false;
>  
>      if (!GetProperty(cx, internals, internals, cx->names().type, &value))
>          return false;
> +    JSLinearString* type = value.toString()->ensureLinear(cx);

Could you put this and the type-testing code in a block, so it's readably clear this variable's only used for ASCII equality tests?  The duration of this variable otherwise is way longer than I'd be willing to trust with unrooting it and leaning entirely on the static-analysis crutch.

@@ +3972,5 @@
>          return false;
>  
>      if (!GetProperty(cx, internals, internals, cx->names().style, &value))
>          return false;
> +    JSLinearString* style = value.toString()->ensureLinear(cx);

Same block-thing.

@@ +4082,3 @@
>      if (!locale)
>          return false;
> +    const char* loc = CaseMappingLocale(locale);

Could you have a helper to compute this, so the JSLinearString* can be clearly minimally safely scoped, rather than existing as garbage to the end of the function?

@@ +4126,3 @@
>      if (!locale)
>          return false;
> +    const char* loc = CaseMappingLocale(locale);

Same helper function again.

Comment 6

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] (I'm baaaaaaack...) from comment #4)
> I am finding the spec algorithm more and more offensive here, in that it's
> not styled
> 
>     1. If |options| is undefined,
>        1. Let usage be "sort".
>        1. ...
>     1. Else
>        1. Let |options| be |ToObject(options)|.
>        1. ...
> 
> but if I ever get that fed up, I probably should just PR it upstream and
> then make such change here.  We're good with this now, of course.

:-)

Comment 7

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] (I'm baaaaaaack...) from comment #5)
> I'm not super-happy about removing unnecessary rooting when the relevant
> variable's scope is tens of lines or more, as a general rule.  It's one
> thing for a few lines with no |cx| usage in sight, but it's very different
> when it's a lot more and you just can't clearly see that at a glance.  Let's
> try to ameliorate that with a few more narrowly-scoped blocks or helper
> functions to minimize unrooted things' scope.

The performance work of the last months may have "corrupted" me a bit, trying to squeeze out additional nano-seconds everywhere. ;-)

> 
> ::: js/src/builtin/Intl.cpp
> @@ +3677,5 @@
> >          return false;
> >  
> >      if (!GetProperty(cx, internals, internals, cx->names().type, &value))
> >          return false;
> > +    JSLinearString* type = value.toString()->ensureLinear(cx);
> 
> Could you put this and the type-testing code in a block, so it's readably
> clear this variable's only used for ASCII equality tests?  The duration of
> this variable otherwise is way longer than I'd be willing to trust with
> unrooting it and leaning entirely on the static-analysis crutch.

Ok, added blocks around the two mentioned unrooted strings and all other unrooted strings which span multiple lines in Intl.cpp

> 
> @@ +4082,3 @@
> >      if (!locale)
> >          return false;
> > +    const char* loc = CaseMappingLocale(locale);
> 
> Could you have a helper to compute this, so the JSLinearString* can be
> clearly minimally safely scoped, rather than existing as garbage to the end
> of the function?

Changed CaseMappingLocale to just take JSString* and then moved the whole linearisation into CaseMappingLocale.

Comment 8

[André Bargull [:anba]]

Attachment: bug1398185-part2-remove-more-extra-rooting.patch

Updated part 2 per review comments, carrying r+.

Comment 9

[André Bargull [:anba]]

Attachment: bug1398185-part1-use-null-proto.patch

Needed to amend part 1 to update a jit-test which was still expecting the old behaviour. Otherwise no changes, therefore carrying r+.

Comment 10

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b304597afc5b7330211fc99f49503fec5e03df6

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/889f583e6e18
Part 1: Use null prototype for default options in Intl objects. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f64bb3bef2d
Part 2: Remove more unnecessary rooting from Intl code. r=Waldo

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/889f583e6e18
https://hg.mozilla.org/mozilla-central/rev/5f64bb3bef2d