Bug 185557

Summary: [INTL] improve efficiency of Intl.NumberFormat formatToParts
Product: WebKit Reporter: Andy VanWagoner <andy>
Component: JavaScriptCoreAssignee: Andy VanWagoner <andy>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, feliziani.emanuele, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Andy VanWagoner 2018-05-11 12:30:34 PDT
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).
Comment 1 Emanuele Feliziani 2019-01-31 23:49:20 PST
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.
Comment 2 Andy VanWagoner 2019-02-01 08:17:26 PST
(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.
Comment 3 Andy VanWagoner 2019-02-01 15:14:40 PST
Created attachment 360910 [details]
Patch
Comment 4 Andy VanWagoner 2019-02-01 15:16:15 PST
I'm hoping this is both faster, and also an easier algorithm to understand & maintain.
Comment 5 Mark Lam 2019-02-04 15:08:51 PST
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.
Comment 6 Andy VanWagoner 2019-02-05 11:16:18 PST
Created attachment 361205 [details]
Patch
Comment 7 Mark Lam 2019-02-05 11:56:32 PST
Comment on attachment 361205 [details]
Patch

Removing r? flag since this patch has already been reviewed.
Comment 8 WebKit Commit Bot 2019-02-05 14:09:38 PST
Comment on attachment 361205 [details]
Patch

Clearing flags on attachment: 361205

Committed r240992: <https://trac.webkit.org/changeset/240992>
Comment 9 WebKit Commit Bot 2019-02-05 14:09:40 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2019-02-05 14:10:31 PST
<rdar://problem/47830639>