js::intl_patternForSkeleton() is very alloc heavy

RESOLVED FIXED in Firefox 55

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: Ehsan, Assigned: eoger)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 3 obsolete attachments)

The deallocations from udatpg_open() can show up in profiles: https://perfht.ml/2no0NOo
Blocks: 1350417
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p1]
Created attachment 8855560 [details] [diff] [review]
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.
Attachment #8855560 - Flags: review?(markh)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED

Comment 2

3 months ago
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
Attachment #8855560 - Flags: review?(markh) → review+

Comment 3

3 months ago
Is gSyncUI a long-living object which may need to invalidate these caches when the time zone (or locale - bug 1349401) changes?
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.
Created attachment 8856067 [details] [diff] [review]
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!
Attachment #8856067 - Flags: review?(markh)
Attachment #8855560 - Attachment is obsolete: true

Comment 6

2 months ago
(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.
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.  :-(
Flags: needinfo?(eoger)
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.
(Assignee)

Comment 9

2 months ago
Created attachment 8858384 [details] [diff] [review]
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
Flags: needinfo?(eoger)
(Assignee)

Comment 10

2 months ago
Not there yet, still some tests failing, do you want me to take over this bug Jeff?
Flags: needinfo?(jwalden+bmo)
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.
Assignee: jwalden+bmo → eoger
Flags: needinfo?(jwalden+bmo)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1f480113d1736d0422ca828bf9c8bcfee7fb965
(Assignee)

Updated

2 months ago
Attachment #8856067 - Attachment is obsolete: true
Attachment #8856067 - Flags: review?(markh)
(Assignee)

Updated

2 months ago
Attachment #8858384 - Attachment is obsolete: true
(Assignee)

Comment 14

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

Comment 15

2 months ago
mozreview-review
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
Attachment #8858908 - Flags: review?(markh) → review+

Comment 16

2 months ago
Pushed by eoger@mozilla.com:
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

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bd0d461eddc1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.