Accurate Array.prototype.flat definition#32131
Accurate Array.prototype.flat definition#32131sandersn merged 5 commits intomicrosoft:masterfrom eilvelia:array-flat
Conversation
|
@typescript-bot test this |
|
Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at ade67c6. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @RyanCavanaugh, I've started to run the parallelized community code test suite on this PR at ade67c6. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @RyanCavanaugh, I've started to run the parallelized Definitely Typed test suite on this PR at ade67c6. You can monitor the build here. It should now contribute to this PR's status checks. |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
I'm not sure, should I merge it? The failed tests don't look related to |
sandersn
left a comment
There was a problem hiding this comment.
I think this needs a synthetic performance test to figure out if the new code is faster or slower. Maybe we can guess just by thinking about the compiler implementation though.
A performance test would look like several thousand calls to flat from the mix of different types supported by the old version, eg ReadonlyArray<ReadonlyArray<U[]>[]>. Plus maybe longer versions of the same thing.
src/lib/es2019.array.d.ts
Outdated
| @@ -1,3 +1,10 @@ | |||
| type Flat<A, D extends number> = { | |||
There was a problem hiding this comment.
we should check for existing types named Flat in some code base -- probably it's fine, but might not be.
There was a problem hiding this comment.
Probably Definitely Typed is a big enough sample -- grepping for class Flat, interface Flat, type Flat should give you (almost?) all of the uses.
There was a problem hiding this comment.
- seen has
class Flat(appropriate for a 3D renderer) - array.prototype.flat has
interface Flat(andFlatMap)
Seen is barely used, but array.prototype.flat usage is quite widespread.
sandersn
left a comment
There was a problem hiding this comment.
Also needs results from a synthetic performance test to see which is faster.
Data from parse/bind/check time of lib.d.ts (without --skipLibCheck) would be nice too, but not necessary. It would be hard to show a significant improvement there.
|
@Bannerets do you have time for a performance test? Otherwise I'd like to close the PR to keep the number of open PRs manageable. |
|
I'm not sure what's the correct way to write synthetic performance tests for types (since I don't know when the compiler can perform caching, etc.). Is there a utility in the typescript repository for this? For example, I wrote this generator: $ cat gen.js
for (let i = 0; i < 4000; i++) {
console.log(
`declare var arr1_${i}: ReadonlyArray<ReadonlyArray<(string | number)[]>[]>;
arr1_${i}.flat(3);
declare var arr2_${i}: ReadonlyArray<ReadonlyArray<number>[]>;
arr2_${i}.flat(2);
declare var arr3_${i}: ReadonlyArray<string[][][][][][]>;
arr3_${i}.flat(4);
declare var arr4_${i}: ReadonlyArray<number[][]>;
arr4_${i}.flat(2);
declare var arr5_${i}: ReadonlyArray<ReadonlyArray<ReadonlyArray<string[]>>>;
arr5_${i}.flat(3);`)
}
$ mv built built-new
$ git checkout 7f1df6e53e
$ npm run build
$ mv built built-old
$ node gen.js > ttest.ts
$ time node ./built-new/local/tsc.js --noEmit --lib es2019.array ttest.ts
61.21 real 63.06 user 0.54 sys
$ time node ./built-old/local/tsc.js --noEmit --lib es2019.array ttest.ts
115.83 real 117.41 user 0.89 sysIt shows the unbelievable 88% increase in performance (I've tried to run it multiple times). |
|
I switched to object literal types instead of string/number because those are fresh on each usage. But with 4000 copies, master now runs out of memory! So I reduced the number to 2000 and observed a ratio that's even better than yours. BeforeAftergen.jsfor (let i = 0; i < 4000; i++) {
console.log(
`declare var arr1_${i}: ReadonlyArray<ReadonlyArray<{ a_${i}: number }[]>[]>;
arr1_${i}.flat(3);
declare var arr2_${i}: ReadonlyArray<ReadonlyArray<{ b_${i}: number }>[]>;
arr2_${i}.flat(2);
declare var arr3_${i}: ReadonlyArray<{ c_${i}: number }[][][][][][]>;
arr3_${i}.flat(4);
declare var arr4_${i}: ReadonlyArray<{ d_${i}: number }[][]>;
arr4_${i}.flat(2);
declare var arr5_${i}: ReadonlyArray<ReadonlyArray<ReadonlyArray<{ e_${i}: number }[]>>>;
arr5_${i}.flat(3);`)
} |
|
The reason is probably that there are 8 overloads, with the shallower-array overloads near the bottom. Conditional type instantiation and overload resolution are both linear, but shallower arrays resolve first with the conditional type. It could be also that conditional type instantiation is more efficient than overload resolution. |
sandersn
left a comment
There was a problem hiding this comment.
Looks much more efficient in practise.
|
I re-ran with |
|
I find it interesting that these typings type FlatArray<Arr, Depth extends number> = {
"done": Arr,
"recur": Arr extends ReadonlyArray<infer InnerArr>
? FlatArray<InnerArr, [-1, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20][Depth]>
: Arr
}[Depth extends -1 ? "done" : "recur"];use the recursive conditional type "trick" that circumvents the restrictions discussed in #26980 and was explicitly warned against by @ahejlsberg and similarly not given the seal of approval by @sandersn. I've used the sort of depth limiting being done here, but always with the understanding that this is murky territory where dragons and other things that go bump in the night reside, and never with the thought that such code would make it into anything standard. So, uh, what's the story here? Should I turn in my badge and service weapon? Should internal affairs be brought in? Does anything make sense anymore? Thanks! |
Fixes #29604 (and #29741).
Examples:
The old declaration returns
any[]for all cases exceptc0,d0and supportsdepthonly up to 7 (this one supports up to 20).