Bug 185557 - [INTL] improve efficiency of Intl.NumberFormat formatToParts
Summary: [INTL] improve efficiency of Intl.NumberFormat formatToParts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy VanWagoner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-05-11 12:30 PDT by Andy VanWagoner
Modified: 2019-02-05 14:10 PST (History)
8 users (show)

See Also:


Attachments
Patch (5.21 KB, patch)
2019-02-01 15:14 PST, Andy VanWagoner
no flags Details | Formatted Diff | Diff
Patch (5.39 KB, patch)
2019-02-05 11:16 PST, Andy VanWagoner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>