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

Revert pattern matching improvements introducing regressions in 3.6.0 #21735

Closed
wants to merge 2 commits into from

Conversation

WojciechMazur
Copy link
Contributor

Reverts #21000 (introducing regression #21569) from 3.6.0
Reverts #18377 single commit 9045834 (introducing regression #21218) from 3.6.0
Done in the same way, as it was done to the LTS branch in #21573 and 3.5.2 in #21734

This reverts PR scala#21000 due to introduced performance regression
This reverts commit 9045834 due to introduced regression
@dwijnand
Copy link
Member

dwijnand commented Oct 9, 2024

Why are we reverting this from 3.6.0?

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Why?

@tgodzik
Copy link
Contributor

tgodzik commented Oct 9, 2024

I think that's due to #21569 regression.

@WojciechMazur
Copy link
Contributor Author

Introduces regression increasing compilation time in #21569 original project from few minutes to almost 4 hours. Personally I'd like to keep all the improvements, but this level of slowdown is unacceptable even though the codebase is quite specific.

@dwijnand
Copy link
Member

dwijnand commented Oct 9, 2024

The fix is correct, and the code that was already slow and is now insanely slow is some pretty crazy code by itself. So I can understand dropping it from a bugfix release, but we shouldn't revert it for a minor release.

@WojciechMazur
Copy link
Contributor Author

I've tried to collect some numbers from the Open CB execution times.
For the majority of projects the compilation time has not changed. There's less than 10 minutes to total OpenCB execution time, which with ~1600 tested takes ~30h, so can be treated as a statical error.
However, I've found out that another project is affected by this change and was unnoticed until now.

https://github.com/andimiller/cats-parse-interpolator was previously compiling in 1m 36s, now it takes 4h 5m.
https://github.com/fommil/jzon was previously taking 2m 55s, now it's 4h as well

@WojciechMazur
Copy link
Contributor Author

I suggest we give the revert of these changes under the vote of today's core meeting.

@WojciechMazur
Copy link
Contributor Author

During the core meeting, we all agreed that #21000 solved multiple, real problems and decided we should not revert it in 3.6.0.

Both of the affected projects were to some extent abusing the Scala type system (via making everything inlined or creating an abnormally large type signature). Instead, we should work on introducing a mechanism that would trigger a warning when pattern match analysis (for reachability or exhaustivity) is taking too much time.
We'd check if we can mitigate the problems of too-long compilation by adjusting the codebase of affected projects.

@WojciechMazur WojciechMazur deleted the revert-3.6.0/21000 branch October 9, 2024 14:50
@dwijnand
Copy link
Member

dwijnand commented Oct 9, 2024

I'm working on that warning now, which we can backport to 3.6.1, or maybe if we have a 3.6.0-RC2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants