patmat tweaks: compiler performance, better error on unsupported pattern #1134

Merged
merged 2 commits into from Aug 14, 2012

Projects

None yet

3 participants

@adriaanm
Member

(rework of #1112)

  • do not catch StackOverflowError: @gkossakowski diagnosed it to be killing compiler performance
    --> limit recursion depth in negationNormalForm in the same way we were already harnessing conjunctiveNormalForm -- configurable using -Dscalac.patmat.analysisBudget=N (N=256 by default)
  • Roland reported hitting the unsupported pattern branch in translatePattern in akka code, so providing some diagnostics and a nicer error message to figure out what's going on

review by @gkossakowski (/cc @paulp)

@gkossakowski
Member

Apart from cleaning up things LGTM.

I think 256 recursion depth should be more than enough. I'd be very worried if we have formulas with more than 256 levels of nesting.

@adriaanm
Member

damn, forgot to push -f -- I already had a version without AnalysisBudget.stackOverflow

@adriaanm adriaanm don't catch StackOverFlowError: kills performance
instead, use a recursion counter in negationNormalForm as in conjunctiveNormalForm
(except we only limit recursion depth, not formula-size
 reuse the same config variable in hopes it'll do for both)
9285bdb
@adriaanm
Member

PLS REBUILD ALL

@adriaanm
Member

I'd say the formulae can get pretty big without cause for concern -- I have a prototype version that uses Sat4J for solving, where 10K formulae is routine. The main cost is my naive NNF/CNF transform. Should use a more efficient approach, but that requires more substantial rewrites to how the formulae are constructed during the analyses. Will work on this for 2.10.1.

@gkossakowski
Member

I know it can get big in length but I'd be surprised if it gets very deeply nested. Or does it?

@adriaanm
Member

alternatives tend to blow up -- so does the encoding of the subtyping relation into binary proposition logic, especially when sealed classes are involved

@gkossakowski
Member

I see. Since you can customize this with an option then I think we are fine with default nesting limit.

I see you pushed cleaned up commit. LGTM.

@adriaanm adriaanm merged commit af1d0a8 into scala:2.10.x Aug 14, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment