Ts-toolbelt: Performance issues with this lib and ramda

[ekilah]

🐞 Bug Report

Performance of DefinitelyTyped/ramda has suffered significantly with a recent upgrade (which relies on this library heavily now)

Describe the bug

Upgrading from @types/[email protected] to @types/[email protected] also upgrades ts-toolbelt from 3.8.4 to 4.7.7. In my project, which uses Ramda frequently but not in any crazy manner, this one change in my project.json doubles my project's TS type checking time from ~25sec to ~60-70sec.

Expected behavior

There is not such a significant slowdown of type-checking when using this library/ @types/ramda.

Additional context

Using the latest TS, 3.6.3

[ekilah]

further testing:

@types/[email protected] is the latest update I can use w/o a significant increase in type-checking times. that version asks for ts-toolbelt@^3.8.75 which resolves to 3.14.0.

@types/[email protected] causes the slowdown, presumably because of the change to ask for ts-toolbelt@^4.3.0 instead (currently resolves to 4.8.23).

I also tried adding a selective dependency resolution to see if the problem arose between 4.3.0 and 4.8.23 or if it came up before/at 4.3.0:

"resolutions": {
    "@types/ramda/ts-toolbelt": "4.3.0"
  }

This locks ts-toolbelt to the earliest version it needs (4.3.0), which still causes slowdowns, so it looks like the change from v3 to v4 is the main culprit. That's about as specific as I've narrowed the issues down so far.

[millsp]

Hi @ekilah, thanks for reporting this. I'll investigate this.

Would you mind listing which ramda functions you use?

(And if possible, the code that is causing the problems)

[ekilah]

@pirix-gh It's a big project so I don't think it comes down to one piece of code over another, so it'd be hard to say exactly what code the issue is manifesting at, but a doubling (actually, a bit more) of type-checking time makes me at least guess it's more to do with how often we use Ramda vs one block of code?

I have a gut feeling the change has to do with the mergeDeepLeft/Right and mergeLeft/Right but that's just because those improved a lot between these versions of ts-toolbelt (I know this from our conversation over on https://github.com/DefinitelyTyped/DefinitelyTyped/pull/38419 ). And, we do use those somewhat often, it looks like:

mergeRight - 48
contains - 27
test - 25
map - 23
pick - 21
equals - 16
filter - 15
repeat - 14
range - 13
prop - 12
forEachObjIndexed - 12
omit - 11
find - 10
sortBy - 10
any - 10
keys - 9
mapObjIndexed - 9
all - 8
values - 8
mergeDeepRight - 7
isNil - 7
take - 6
forEach - 6
nth - 6
mergeDeepLeft - 5
sortWith - 5
isEmpty - 5
flatten - 5
takeLast - 5
last - 5
mergeDeepWith - 5
identity - 5
reduce - 4
clone - 4
chain - 4
descend - 4
groupBy - 4
reject - 4
ascend - 3
invert - 3
fromPairs - 3
times - 2
splitAt - 2
clamp - 2
has - 2
splitEvery - 2
includes - 2
recipientName - 2
concat - 1
invites - 1
findLastIndex - 1
findIndex - 1
indexOf - 1
pipe - 1
reverse - 1
max - 1
toLower - 1
partition - 1
remove - 1
takeWhile - 1
anyPass - 1
compose - 1
complement - 1
dissoc - 1
none - 1
prepend - 1
evolve - 1
insert - 1
without - 1
toPairs - 1
not - 1
split - 1
pickBy - 1
drop - 1

[ekilah]

I could somewhat easily remove all of our uses of mergeRight and see what that does to the time it takes for the type-checker to run, since that function can just be replaced with the object spread operator - if you think that sort of test would be useful / if you agree that these merge functions might be the core of the problem, I can give that a go later, it'll just take some time to change all 48 uses hah.

[millsp]

Thanks! This is what I was asking for on gitter. I also think it is to do with merge.

[ekilah]

Summarizing our findings so far:

using yarn run tsc --extendedDiagnostics, we saw that the Check time was around 52 seconds long in my project with these newest types for ramda. By only changing the return type on mergeRight we saw that time go down to 26 seconds!

So, I started to narrow down the 48 usages of mergeRight to see which of them were causing issues. While removing several usages initially, I was seeing mostly negligible differences in "Check time", with a reduction of roughly 4 seconds after 15 usages were removed.

Then, I was able to shave 15 seconds or so of this off just by removing 7 usages that all looked like this:

             <FontAwesomeIcon
               icon={faArrowCircleUp}
               className={className}
-              style={R.mergeRight(
-                {
-                  width: '17px',
-                  height: '17px',
-                },
-                style
-              )}
+              style={{
+                width: '17px',
+                height: '17px',
+                ...style,
+              }}
             />
           )
         }

The style prop on <FontAwesomeIcon> and the local var style (a local function parameter style: React.CSSProperties = {}) are of type React.CSSProperties which is quite a large type with lots of keys...

[millsp]

It was actually a little mistake, with big consequences. A mapped type in a mapped type... So for an object of 500 properties, it escalated from 500 iterations to 250500.

Thanks to this, I went through the whole codebase and optimized other types that are used everywhere (like ...Keys types). This should provide better performance overall (but not tremendous improvements).

Let me know of the new statistics/diagnostics for your project, and I hope that webpack module will do the job for you & your org.

[ekilah]

today ([email protected]):

Files:                       1735
Lines:                     275980
Nodes:                     988213
Identifiers:               318230
Symbols:                   595394
Types:                     292563
Memory used:              741845K
Assignability cache size:  184768
Identity cache size:        11600
Subtype cache size:         24148
I/O Read time:              0.51s
Parse time:                 1.71s
Program time:               3.69s
Bind time:                  1.27s
Check time:                27.09s
transformTime time:         2.12s
Source Map time:            0.44s
commentTime time:           0.46s
I/O Write time:             0.75s
printTime time:             6.81s
Emit time:                  6.81s
Total time:                38.86s
✨  Done in 39.62s.

which is great. I can save 3 more seconds by removing those R.mergeRights still though, so I'll still leave them out, but that's a major improvement nonetheless.

[millsp]

Hi @ekilah, is there a regression again?

[millsp]

Oh I see, not a regression. I'll try to allocate some time in the future to see if this type can be shortened. Not sure it can, though. I'll let you know

[ekilah]

oh, no, sorry, I just wanted to show the conclusion for anyone seeing this issue later!

[millsp]

Just to ping, I successfully improved performance of MergeUp (again) as well as type correctness (where it had an unsound behavior for one edge-case).

So if you feel like you want to update, feel free to do it. However, I don't think the performance will change very much, it's more about the better (100%) type-safety.