Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accurate Array.prototype.flat definition #32131

Merged
merged 5 commits into from
Apr 7, 2020
Merged

Accurate Array.prototype.flat definition #32131

merged 5 commits into from
Apr 7, 2020

Conversation

eilvelia
Copy link
Contributor

@eilvelia eilvelia commented Jun 27, 2019

Fixes #29604 (and #29741).

Examples:

const a0 = [1, [2]].flat() // const a0: number[]
const a1 = [[1], 'a'].flat(1) // const a1: (string | number)[]
const n: number = 1
const a2 = [1, [2]].flat(n) // const a2: (number | number[])[]
const b0 = [[1], ['a']].flat(1) // const b0: (string | number)[]
const c0 = [
  [[1]],
  [[2]]
].flat(2) // const c0: number[]
const arr = [[[1, [2, [3, [4]]]]]]
const d0 = arr.flat(2) // const d0: (number | (number | (number | number[])[])[])[]
const d1 = arr.flat(n) // const d1: (number | number[] | (number | number[])[] | (number | (number | number[])[])[] | (number | (number | (number | number[])[])[])[] | (number | (number | (number | number[])[])[])[][])[]

The old declaration returns any[] for all cases except c0, d0 and supports depth only up to 7 (this one supports up to 20).

@RyanCavanaugh
Copy link
Member

@typescript-bot test this
@typescript-bot run dt
@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 16, 2019

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.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 16, 2019

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.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 16, 2019

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.

@RyanCavanaugh RyanCavanaugh self-assigned this Jul 16, 2019
@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

@eilvelia
Copy link
Contributor Author

I'm not sure, should I merge it? The failed tests don't look related to Array#flat.

@sandersn sandersn added the For Milestone Bug PRs that fix a bug with a specific milestone label Feb 1, 2020
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
src/lib/es2019.array.d.ts Outdated Show resolved Hide resolved
@@ -1,3 +1,10 @@
type Flat<A, D extends number> = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should check for existing types named Flat in some code base -- probably it's fine, but might not be.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably Definitely Typed is a big enough sample -- grepping for class Flat, interface Flat, type Flat should give you (almost?) all of the uses.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • seen has class Flat (appropriate for a 3D renderer)
  • array.prototype.flat has interface Flat (and FlatMap)

Seen is barely used, but array.prototype.flat usage is quite widespread.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/lib/es2019.array.d.ts Outdated Show resolved Hide resolved
@sandersn
Copy link
Member

@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.

@eilvelia
Copy link
Contributor Author

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 sys

It shows the unbelievable 88% increase in performance (I've tried to run it multiple times).

@sandersn
Copy link
Member

sandersn commented Apr 7, 2020

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.

Before

real	0m19.253s
user	0m24.197s
sys	0m0.667s

After

real	0m9.005s
user	0m11.581s
sys	0m0.245s

gen.js

for (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);`)
}

@sandersn
Copy link
Member

sandersn commented Apr 7, 2020

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.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much more efficient in practise.

@sandersn
Copy link
Member

sandersn commented Apr 7, 2020

I re-ran with --diagnostics and noticed that subsequent runs of the old compiler are much faster. I averaged four runs (not because that's a magic number, but because that's how much patience I had) and the average is 9.15 vs 9.12. So I think actually the two are about the same.

@sandersn sandersn merged commit c47aca0 into microsoft:master Apr 7, 2020
@jcalz
Copy link
Contributor

jcalz commented May 1, 2020

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!

@dead-claudia
Copy link

@jcalz Ironically, this PR was authored by @sandersn... 😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Wrongly infers any[] from Array.prototype.flat, Array.prototype.concat
6 participants