hauntsaninja / mypy_primer

Run mypy and pyright over millions of lines of code
MIT License
55 stars 29 forks source link

Improve performance reports #81

Closed AlexWaygood closed 1 year ago

AlexWaygood commented 1 year ago
hauntsaninja commented 1 year ago

Thanks, this looks good! Agreed that we're only going to be able to detect really large shifts. That said, I'd like to hold off a week or two before merging, I'm finding it interesting to see how much noise there is.

AlexWaygood commented 1 year ago

This has some big merge conflicts now -- are you still interested in this/should I rebase? :)

JelleZijlstra commented 1 year ago

I feel like I see noisy output from the performance report fairly regularly and I basically trained myself to ignore it.

AlexWaygood commented 1 year ago

Yeah, I agree that there have been PRs where we can be confident that <10s speedups were real, because they were very consistent in the mypy_primer reports. However I feel like I've seen more PRs with random noise from the performance reports, and I think those can be pretty confusing to new contributors (especially at typeshed, where it's pretty unlikely that something will have a significant performance impact unless somebody's adding a massive Union somewhere).

So I guess I can see it both ways? Maybe the ideal would be if the artifact contained a complete performance report, but the comment posted on PRs filtered out any differences <10s. That would be more complex to implement, of course.

Whatever the case, I do think it would be good to clarify that it's the typechecking of these projects that's getting faster/slower — not sure the wording is ideal at the moment 😄

Akuli commented 1 year ago

I think those can be pretty confusing to new contributors (especially at typeshed, where it's pretty unlikely that something will have a significant performance impact

IMO we should disable performance reports in typeshed, maybe with a command-line flag to mypy_primer? We don't really care if one package becomes slightly slower, as long as it is reasonably fast overall. This is different in mypy, because changes in mypy affect all/most packages, and also pile up unlike improvements to different packages.

unless somebody's adding a massive Union somewhere).

Mypy handles big unions reasonably fast in most practical cases. There is a fast path that skips the bottleneck, and it has been expanded to cover pretty much anything that comes up in practice.

AlexWaygood commented 1 year ago

Mypy handles big unions reasonably fast in most practical cases. There is a fast path that skips the bottleneck, and it has been expanded to cover pretty much anything that comes up in practice.

Hmm, well, every typeshed sync over at mypy at the moment, we religiously cherry-pick a commit to revert a change I made to typeshed a while back that allegedly caused a large performance regression for mypy due to the use of a big union: https://github.com/python/mypy/blob/e14cddbe31c9437502acef022dca376a25d0b50d/misc/sync-typeshed.py#L183

See e.g. the third commit in https://github.com/python/mypy/pull/15165

If mypy really has no performance problems with big unions, we should probably stop doing that :D

hauntsaninja commented 1 year ago

Consensus seems it's too noisy, so let's up the threshold to 10s (like this PR does), and see how we feel about noise?

I've definitely caused huge perf regressions with typeshed changes, so I think there's value to it in typeshed if the number can be believable.

I agree that it makes sense to target wording at infrequent contributors, maybe something like "in a single noisy sample, checking was 1.14x slower"

re the sum literal change: Could be worth trying out again? I've lost track of what it was trying to solve though and I haven't yet spotted what is undesirable in mypy's current behaviour

Akuli commented 1 year ago

Apparently I was wrong about big unions :)

They used to be really broken. Specifically, there was a nested for loop that did n^2 iterations for a union of n members, so with some AWS related stubs with a union of 200 strings, it took a few minutes to type-check a very short file. But I didn't realize this isn't the only union slowness bug.

hauntsaninja commented 1 year ago

Yeah, I improved two more union perf things in mypy recently, so it might be better now

AlexWaygood commented 1 year ago

Cool I'll update this PR based on the consensus here!