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

Andy VanWagoner
Reported 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).
Attachments
Patch (5.21 KB, patch)
2019-02-01 15:14 PST, Andy VanWagoner 		no flags
DetailsFormatted DiffDiff
Patch (5.39 KB, patch)
2019-02-05 11:16 PST, Andy VanWagoner 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Emanuele Feliziani
Comment 1 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.
Andy VanWagoner
Comment 2 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.
Andy VanWagoner
Comment 3 2019-02-01 15:14:40 PST
Created attachment 360910 [details] Patch
Andy VanWagoner
Comment 4 2019-02-01 15:16:15 PST
I'm hoping this is both faster, and also an easier algorithm to understand & maintain.
Mark Lam
Comment 5 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.
Andy VanWagoner
Comment 6 2019-02-05 11:16:18 PST
Created attachment 361205 [details] Patch
Mark Lam
Comment 7 2019-02-05 11:56:32 PST
Comment on attachment 361205 [details] Patch Removing r? flag since this patch has already been reviewed.
WebKit Commit Bot
Comment 8 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>
WebKit Commit Bot
Comment 9 2019-02-05 14:09:40 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 10 2019-02-05 14:10:31 PST
<rdar://problem/47830639>
Note You need to log in before you can comment on or make changes to this bug.