1350613 - js::intl_patternForSkeleton() is very alloc heavy

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[(no longer active)]

The deallocations from udatpg_open() can show up in profiles: https://perfht.ml/2no0NOo

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Don't repeatedly create DateTimeFormat instances for the exact same set of options, but instead use cached formatters

udatpg_open should be a one-time cost per DateTimeFormat object instance.  And the intent of DateTimeFormat, is that users should create *one* instance for a particular options set, then use it repeatedly.  Creating a new instance for every format operation, or using Date.prototype.toLocale{Date,Time,}String for formatting a large number of dates all the same way, is a misuse of the API.

It looks like Sync code may well be wrongly doing expensive computations repeatedly -- https://searchfox.org/mozilla-central/search?q=tolocaledatestring points out a few uses of toLocaleDateString in Sync code, and the profile linked in comment 0, if I invert it, seems to point to that code.  Let's try fixing it to do things the right way.  This is untested, but I'm pretty sure it does what's desired here.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8855560 [details] [diff] [review]
Don't repeatedly create DateTimeFormat instances for the exact same set of options, but instead use cached formatters

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

LGTM

Comment 3

[André Bargull [:anba]]

Is gSyncUI a long-living object which may need to invalidate these caches when the time zone (or locale - bug 1349401) changes?

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

I do believe that this cache will have to be invalidate on locales change, yes. But we're not yet at the point where we're migrating our codebase to handle language switching on fly, so I'm ok with this landing as-is.

I'll be fixing all Intl formatter cache objects when we get to that.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Updated, responding to tryserver complaints

Try wasn't happy about the patch as posted, 1) for the failure to |this.|-qualify the getter functions (bah, too much time in C++ lately), but also because 2) apparently this code *relies* on |Date.prototype.toLocaleDateString| returning |"Invalid Date"| when a |Date| with time value (or a primitive number value?) of |NaN| is specified.  (|Intl.DateTimeFormat.prototype.format| throws a |RangeError| when passed such a value.)

To fix this, I added code that checks for |NaN| and returns that |"Invalid Date"| string manually.  This fixes the problem:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b37e15916842dc168e8f6d7c3ce53823870c334

but this is a very fruity way to handle things.  Particularly, |isNaN(x)| is |var y = ToNumber(x); return y !== y;| which *coerces* the argument to a number: I don't know if the argument is always a |Date|, or always a primitive number, so I can't use the better-typed |Number.isNaN| that returns true *only* for the primitive number |NaN|, and not for object values that convert to |NaN| by happenstance.  (Generally |Number.isNaN| is much preferred to |isNaN| for this reason.)

*Is* this function only passed Dates, or primitive numbers, or something that would let me not use the very-broad |isNaN| function?  Supposing I do need the full generality, is this permissible/desirable as a fix?  And should I file a followup for someone to figure out why/how our code invokes a formatting function on invalid date values, returning an un-localized string in the process?

Basically, I've stumbled upon untyped madness in this JS.  Tell me the proper way through it!

Comment 6

[Mark Hammond [:markh] [:mhammond]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> 2) apparently this code *relies* on
> |Date.prototype.toLocaleDateString| returning |"Invalid Date"| when a |Date|
> with time value (or a primitive number value?) of |NaN| is specified. 
> (|Intl.DateTimeFormat.prototype.format| throws a |RangeError| when passed
> such a value.)

Where is that assumption? dxr shows many places "Invalid Date" seems to be assumed, but none look they would be hit here, and there's no intention of this happening in practice. IOW, I'm wondering if a test needs to be fixed instead?

Do you still have the link to the failing try push?

> I don't know if the argument is always a |Date|

The argument should always be a date - there's only 3 non-test callers of this and only 2 in tests.

If you have that failing try push I'm sure :eoger can help us work out what is going wrong here.

Comment 7

[Jeff Walden [:Waldo]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37ede8e5ead1f27c900594b0d79c829db953c939&selectedJob=89432375 is one such push -- the failure messages in the summary don't indicate it, but if you grunge through earlier parts of the log in, e.g. the Linux opt bc1 failure, you can find bits of fail.

I should also note that there's some trickiness to all this because, if memory serves, one caller of this junk encloses the call in a try-catch and swallows the thrown exception.  :-(

Comment 8

[Jeff Walden [:Waldo]]

I should say that I don't know the dependency is on "Invalid Date" exactly, but maybe rather on a string being returned and it not throwing.

Comment 9

[Edouard Oger [:eoger]]

Attachment: bug-1350613.patch

I corrected the first patch, we were missing the client.lastModified property in tests.
Let's see:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=23a6e52435f6a256d92b8b1b1a8b0edd6b89d416

Comment 10

[Edouard Oger [:eoger]]

Not there yet, still some tests failing, do you want me to take over this bug Jeff?

Comment 11

[Jeff Walden [:Waldo]]

Sure -- I've got a big project on my plate and a bit of a vacation looming soon, so I'm a little pressed for time.

I will say, however, that the equities seem to me to favor landing the patch I have here, then eliminating the cases with bad Dates flowing in in a followup.

Comment 12

[Edouard Oger [:eoger]]

Attachment: Bug 1350613 - Don't repeatedly create DateTimeFormat instances for the exact same set of options, but instead use cached formatters.

Review commit: https://reviewboard.mozilla.org/r/130892/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130892/

Comment 13

[Edouard Oger [:eoger]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1f480113d1736d0422ca828bf9c8bcfee7fb965

Comment 14

[Edouard Oger [:eoger]]

Try is green \o/, waiting on markh's r+ to land this.

Comment 15

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8858908 [details]
Bug 1350613 - Don't repeatedly create DateTimeFormat instances for the exact same set of options, but instead use cached formatters.

https://reviewboard.mozilla.org/r/130892/#review133612

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd0d461eddc1
Don't repeatedly create DateTimeFormat instances for the exact same set of options, but instead use cached formatters. r=markh

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bd0d461eddc1