Speed up toLocaleString ~2.5x by optimizing creation of Intl.DateTimeFormat instances #1651
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1651 +/- ##
===========================================
- Coverage 95.00% 72.79% -22.21%
===========================================
Files 19 18 -1
Lines 10726 4838 -5888
Branches 1716 1058 -658
===========================================
- Hits 10190 3522 -6668
- Misses 523 1030 +507
- Partials 13 286 +273
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
polyfill/package.json
Outdated
| @@ -8,7 +8,7 @@ | |||
| "types": "index.d.ts", | |||
| "scripts": { | |||
| "coverage": "c8 report --reporter html", | |||
| "test": "node --no-warnings --experimental-modules --icu-data-dir node_modules/full-icu --loader ./test/resolve.source.mjs ./test/all.mjs", | |||
| "test": "time node --no-warnings --experimental-modules --icu-data-dir node_modules/full-icu --loader ./test/resolve.source.mjs ./test/all.mjs", | |||
There was a problem hiding this comment.
Will do. If OK with you, I'd like to leave it in the other repo. Especially as we start attracting new contributors over there, I'd like to instill awareness of perf regressions from everyone contributing code over there. Unless it hurts something?
There was a problem hiding this comment.
I don't suppose it hurts, but I'm slowly removing these tests anyway, so their runtime doesn't matter much here. If you want to think about ways to speed up the 262 tests, that'd help me more :)
polyfill/test/intl.mjs
Outdated
| toJSON() { | ||
| if (++this.calls > 1) throw new RangeError('prop read twice'); | ||
| return value; |
There was a problem hiding this comment.
Should this ever be called?
There was a problem hiding this comment.
Good catch. An early version of this PR used a JSON round-trip as a cheap way to do a deep copy of the input parameters, so this was previously useful. Now it's implemented differently so this toJSON test code will never run. I'll remove it, unless you think there's value in leaving it around in case some future implementation decides to use JSON.stringify on the inputs.
There was a problem hiding this comment.
I'm happy to keep it and add valueOf as well, or just drop it.
There was a problem hiding this comment.
I just dropped it for simplicity. We already have to revisit this per js-temporal/temporal-polyfill#12 (comment) so any improvement in multi-call/proxy-calll tests could happen then.
I think we're good to merge. It's pretty late for me so I'm going to bed soon. If I can't stay up until the Test262 run finishes, I'll merge in the morning if you don't do it first. Then I can go on vacation! :-)
Speed up toLocaleString ~2.5x by optimizing creation of Intl.DateTimeFormat instances. Port of js-temporal/temporal-polyfill#12
justingrant
force-pushed
the
speedup-tolocalestring
branch
from
da763e0 to
a69e0fd
Compare
Port of js-temporal/temporal-polyfill#12