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

Fix bad performance on complex patmat #9522

Merged
merged 1 commit into from Mar 3, 2021

Conversation

dwijnand
Copy link
Member

@dwijnand dwijnand commented Feb 25, 2021

AnalysisBudget.maxDPLLdepth is already working to limit the initial SAT
solving. But given enough unassigned symbols, like the test case, the
compiler can end up spending the rest of eternity and all its memory
expanding the model. So apply the limit where it hurts most (the
cartesian product part).

The ordered sets and various sortings, instead, are to stabilise the
results.

Fixes scala/bug#12237

@dwijnand
Copy link
Member Author

The ordered sets, instead, are necessary for stable results.

Looks like test/files/neg/t7020.check isn't stable across bootstrapped/non-bootstrapped and I don't know why.

@dwijnand dwijnand force-pushed the complex-patmat branch 2 times, most recently from 1d5a9fd to 71535b6 Compare February 26, 2021 20:13
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

LGTM. Maybe use more LinkedHashSet instead of ListSet, as this is all compiler internal?

@dwijnand
Copy link
Member Author

dwijnand commented Mar 3, 2021

Switch the one method-local only ListSet to a mutable.LinkedHashSet and I made the cartesian product return early by making it a tailrec loop. The rest other ListSets cross file or module boundaries so I'd rather keep them immutable. cc @retronym

AnalysisBudget.maxDPLLdepth is already working to limit the initial SAT
solving.  But given enough unassigned symbols, like the test case, the
compiler can end up spending the rest of eternity and all its memory
expanding the model.  So apply the limit where it hurts most (the
cartesian product part).

The ordered sets and various sortings, instead, are to stabilise the
results.
@dwijnand dwijnand merged commit e2990bf into scala:2.13.x Mar 3, 2021
@dwijnand dwijnand deleted the complex-patmat branch March 3, 2021 11:40
@SethTisue
Copy link
Member

at #8830 (comment) @lrytz wrote:

(t7020.scala is flaky currently, likely since #9522)

I've noticed it failing in various places as well (on random PRs, on our Windows Jenkins)

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