Bug 185557 – [INTL] improve efficiency of Intl.NumberFormat formatToParts

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

[Andy VanWagoner]

unum_formatDoubleForFields gives fields that are nested, and in no particular order. The initial implementation transforming those into the list of number format parts is O(N^2), but can theoretically be done in O(N log N).

[Emanuele Feliziani]

Is this bug the reason why this is enabled in Safari Technology Preview, but not in the current iOS 12.2 beta?

It would be great if formatToParts got released in 12.2 when it hits the public.

[Andy VanWagoner]

(In reply to Emanuele Feliziani from comment #1)
> Is this bug the reason why this is enabled in Safari Technology Preview, but
> not in the current iOS 12.2 beta?
> 
> It would be great if formatToParts got released in 12.2 when it hits the
> public.

My best guess is that this is not the blocker for inclusion in Safari. iirc, Apple wants to minimize the differences in Safari versions across OS versions, which have different ICU libraries available. Until all of the supported OS versions can enable the feature, it probably won't make the cut.

If I'm wrong, I'd be happy to be corrected by an Apple representative. If this is a blocker, I can find some time to fix it.

[Andy VanWagoner]

Created attachment 360910 [details]
Patch

[Andy VanWagoner]

I'm hoping this is both faster, and also an easier algorithm to understand & maintain.

[Mark Lam]

Comment on attachment 360910 [details]
Patch

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

r=me with suggested improvements.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:513
> +    auto literalField = std::make_pair(-1, resultLength);

Instead of using a literal -1 like this, can you give it a name e.g.
    static const int32_t defaultFieldType = -1;
    auto literalField = std::make_pair(defaultFieldType, resultLength);

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:514
> +    Vector<std::pair<int32_t, int32_t>> fields(resultLength, literalField);

Why not use a struct instead of a pair here?

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:544
> +        auto partType = fieldType < 0 ? literalString : jsString(&exec, partTypeString(UNumberFormatFields(fieldType), value));

nit: use fieldType == defaultFieldType instead of fieldType < 0.

[Andy VanWagoner]

Created attachment 361205 [details]
Patch

[Mark Lam]

Comment on attachment 361205 [details]
Patch

Removing r? flag since this patch has already been reviewed.

[WebKit Commit Bot]

Comment on attachment 361205 [details]
Patch

Clearing flags on attachment: 361205

Committed r240992: <https://trac.webkit.org/changeset/240992>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47830639>