-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Use concat instead of spread in concatenate #58946
base: main
Are you sure you want to change the base?
Conversation
@typescript-bot perf test this |
@jakebailey, the perf run you requested failed. You can check the log here. |
nodejs.org outage of all things; need to look into retrying those requests. @typescript-bot perf test this |
@jakebailey, the perf run you requested failed. You can check the log here. |
Jeeze, everything's breaking today. |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Less memory across the board, it seems. The rest is surely noise... @typescript-bot perf test this faster |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
That's a remarkably large RAM reduction for such a simple change. |
Yeah, I'm just not sure how to rationalize the 2% slowdown in xstate (which was in both benchmark results). |
Hm, that one's the only statistically significant one, but the other check benchmarks also seem slightly worse. |
For my own curiosity, can you try a possibly-silly experiment where you effectively reimplement let result = [];
for (let i = 0; i < array1.length; i++) result.push(array1[i]);
for (let i = 0; i < array2.length; i++) result.push(array2[i]);
return result; |
could preallocating the array's size be helpful in the suggestion above? |
To be honest, if you call |
Nope, preallocation actually doesn't help at all. When you do something like I can try the manual push. |
From some microbenchmarking on node 18, the scaling varies wildly with array size (on smaller arrays concat shows a stronger lead than for large arrays, but I wasn't able to find any regression). Also worth pointing out that there is significant savings when concatenating more than 2 arrays to do them all at once instead of a pair at a time. i.e. a1.concat(a2,a3);
// or, faster still
[].concat(a1, a2, a3); is much faster than a1.concat(a2).concat(a3); |
@typescript-bot perf test this faster |
I tried to find an example where we concat'd more than once and didn't seem to find any, unfortunately. |
I find three matches for TypeScript/src/compiler/binder.ts Line 1667 in beb375a
Not sure if any of those callers are the hot paths for concatenate, though. That's a question for the profiler. |
Hm, yeah, those all may be hot. |
My largest project (90s build time) only spends 37ms in concatenate according to the profiler, so I really want to know how the perf test hit a >1% regression from this. |
The test case is just https://github.com/statelyai/xstate, if you're interested. |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Once again, 2% in xstate... Going to try the append version instead. |
@typescript-bot perf test this faster |
FWIW this change is totally wrong because |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Just for the sake of noise, can you test what happens if you make this change entirely cosmetic (e.g. just add a comment to the previous implementation) and rerun the perf test? |
@typescript-bot perf test this faster |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Yeah, the 2 percent was real. |
Spitballing, but could the change be causing an extra garbage collection during the test? i.e. something that would eventually happen anyway (and maybe take more time) but happens to trigger earlier with this pattern. |
That doesn't feel right, just because these all have reduced memory usage, which implies that GC is actually doing less work overall... This may take some profiling to figure out. |
@typescript-bot perf test this |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Hm, forgot about xstate being bad with the original change :( |
Per #58873 (comment) (Thanks @dmichon-msft)
Benchmarked these, and yeah concat is a lot faster.