Space leak in QuickCheck test #450

Closed
jacereda opened this Issue May 6, 2016 · 16 comments

Comments

Projects
None yet
2 participants
@jacereda
Contributor

jacereda commented May 6, 2016

The error:

shake-test: Stack space overflow: current size 33568 bytes.

Reducing the number of QuickCheck tests to 500 makes it pass.

@jacereda

This comment has been minimized.

Show comment
Hide comment
@jacereda

jacereda May 6, 2016

Contributor

Forgot to say that I'm building with ghc 8 in case it matters.

Contributor

jacereda commented May 6, 2016

Forgot to say that I'm building with ghc 8 in case it matters.

@ndmitchell

This comment has been minimized.

Show comment
Hide comment
@ndmitchell

ndmitchell May 6, 2016

Owner

Does stubbing out the property still fail? Eg could it be a Mac specific QuickCheck bug? To stub out properly you would need the same type and some kind of deep seq to force the input.

Owner

ndmitchell commented May 6, 2016

Does stubbing out the property still fail? Eg could it be a Mac specific QuickCheck bug? To stub out properly you would need the same type and some kind of deep seq to force the input.

@jacereda

This comment has been minimized.

Show comment
Hide comment
@jacereda

jacereda May 6, 2016

Contributor

Do you mean something like this?

Success{} <- quickCheckWithResult stdArgs{maxSuccess=1000} $ \(Pattern p) (Path x) -> p `deepseq` x `deepseq` (property True)

That passes with -K1K

Contributor

jacereda commented May 6, 2016

Do you mean something like this?

Success{} <- quickCheckWithResult stdArgs{maxSuccess=1000} $ \(Pattern p) (Path x) -> p `deepseq` x `deepseq` (property True)

That passes with -K1K

@ndmitchell

This comment has been minimized.

Show comment
Hide comment
@ndmitchell

ndmitchell May 6, 2016

Owner

Can you reinstate the property but print p and x before so we can see which value it fails on.

Owner

ndmitchell commented May 6, 2016

Can you reinstate the property but print p and x before so we can see which value it fails on.

@jacereda

This comment has been minimized.

Show comment
Hide comment
@jacereda

jacereda May 6, 2016

Contributor

With print (p,x), this is the last pair printed:

("b*\\/**ab//b\\\\\\b\\a\\b*\\b/baab\\b/*a///\\b\\\\/b/*/aaaa*b/b\\b\\\\a\\*bb/*a//\\///\\/**bb\\/aab//\\\\\\**","\\\\/baa\\/ab\\a/\\ab\\a\\/\\aaa///\\ab///\\a\\bb/b\\a/\\/\\/\\///a\\abbb/\\/ab\\//baab/\\\\//a\\a\\a\\//b\\/b\\\\\\/\\/")
Contributor

jacereda commented May 6, 2016

With print (p,x), this is the last pair printed:

("b*\\/**ab//b\\\\\\b\\a\\b*\\b/baab\\b/*a///\\b\\\\/b/*/aaaa*b/b\\b\\\\a\\*bb/*a//\\///\\/**bb\\/aab//\\\\\\**","\\\\/baa\\/ab\\a/\\ab\\a\\/\\aaa///\\ab///\\a\\bb/b\\a/\\/\\/\\///a\\abbb/\\/ab\\//baab/\\\\//a\\a\\a\\//b\\/b\\\\\\/\\/")
@jacereda

This comment has been minimized.

Show comment
Hide comment
@jacereda

jacereda May 6, 2016

Contributor

I have reduced the problem to:

Success{} <- quickCheckWithResult stdArgs{maxSuccess=1000} $ \(Pattern p) (Path x) -> label "foo" True
Contributor

jacereda commented May 6, 2016

I have reduced the problem to:

Success{} <- quickCheckWithResult stdArgs{maxSuccess=1000} $ \(Pattern p) (Path x) -> label "foo" True
@jacereda

This comment has been minimized.

Show comment
Hide comment
@jacereda

jacereda May 6, 2016

Contributor

Changing label "foo" True to property True terminates properly. I guess it's normal since quickcheck is collecting those labels. Isn't reducing maxSuccess to 500 an option here? Would be good to know why you don't get this same error though...

Contributor

jacereda commented May 6, 2016

Changing label "foo" True to property True terminates properly. I guess it's normal since quickcheck is collecting those labels. Isn't reducing maxSuccess to 500 an option here? Would be good to know why you don't get this same error though...

@ndmitchell

This comment has been minimized.

Show comment
Hide comment
@ndmitchell

ndmitchell May 7, 2016

Owner

If you change it to \(p :: Double) (x :: Double) do you still get the overflow? What version of QuickCheck are you using?

Owner

ndmitchell commented May 7, 2016

If you change it to \(p :: Double) (x :: Double) do you still get the overflow? What version of QuickCheck are you using?

@ndmitchell

This comment has been minimized.

Show comment
Hide comment
@ndmitchell

ndmitchell May 7, 2016

Owner

Reducing maxSuccess to 500 is certainly an option, but I'd like to understand what's causing it first. It seems something has a space leak, and from where you are getting to, that something is looking increasingly like QuickCheck, and thus would be good to fix upstream.

Owner

ndmitchell commented May 7, 2016

Reducing maxSuccess to 500 is certainly an option, but I'd like to understand what's causing it first. It seems something has a space leak, and from where you are getting to, that something is looking increasingly like QuickCheck, and thus would be good to fix upstream.

@jacereda

This comment has been minimized.

Show comment
Hide comment
@jacereda

jacereda May 7, 2016

Contributor

QuickCheck version is 2.8.2, but I found the reason.
I was building with stack test --profile. As soon as I changed my build command to stack test the problem disappeared.
Is there some way to detect profiling in cabal so a larger stack can be used there?

Contributor

jacereda commented May 7, 2016

QuickCheck version is 2.8.2, but I found the reason.
I was building with stack test --profile. As soon as I changed my build command to stack test the problem disappeared.
Is there some way to detect profiling in cabal so a larger stack can be used there?

@ndmitchell

This comment has been minimized.

Show comment
Hide comment
@ndmitchell

ndmitchell May 7, 2016

Owner

Thanks, I'll try and reproduce later. If profile is the culprit it's probably a space leak in QC that gets fixed by optimisation. Should still be quite easy to track down and fix.

Owner

ndmitchell commented May 7, 2016

Thanks, I'll try and reproduce later. If profile is the culprit it's probably a space leak in QC that gets fixed by optimisation. Should still be quite easy to track down and fix.

@ndmitchell

This comment has been minimized.

Show comment
Hide comment
@ndmitchell

ndmitchell May 7, 2016

Owner

OK, I can reproduce this in QuickCheck alone, so nothing to do with Shake. Will track it down there.

Owner

ndmitchell commented May 7, 2016

OK, I can reproduce this in QuickCheck alone, so nothing to do with Shake. Will track it down there.

@ndmitchell ndmitchell referenced this issue in nick8325/quickcheck May 7, 2016

Merged

Fix a spaceleak with label function #93

@ndmitchell

This comment has been minimized.

Show comment
Hide comment
@ndmitchell

ndmitchell May 7, 2016

Owner

Space leak diagnosed and patched, pull request at nick8325/quickcheck#93.

Owner

ndmitchell commented May 7, 2016

Space leak diagnosed and patched, pull request at nick8325/quickcheck#93.

@ndmitchell

This comment has been minimized.

Show comment
Hide comment
@ndmitchell

ndmitchell May 7, 2016

Owner

My thoughts are if this gets patched quickly, do nothing to Shake, if not, remove the label call until a release is made.

Owner

ndmitchell commented May 7, 2016

My thoughts are if this gets patched quickly, do nothing to Shake, if not, remove the label call until a release is made.

@ndmitchell ndmitchell changed the title from FilePattern test suite fails on Mac to Space leak in QuickCheck test May 7, 2016

@jacereda

This comment has been minimized.

Show comment
Hide comment
@jacereda

jacereda May 7, 2016

Contributor

Great, thanks.

Contributor

jacereda commented May 7, 2016

Great, thanks.

ndmitchell added a commit that referenced this issue May 7, 2016

@ndmitchell

This comment has been minimized.

Show comment
Hide comment
@ndmitchell

ndmitchell May 7, 2016

Owner

In fact, no point even leaving this open. The label coming out of QuickCheck is of marginal interest at best. Next time I care what the value is I'll reenable it, but until then we can leave it off.

Owner

ndmitchell commented May 7, 2016

In fact, no point even leaving this open. The label coming out of QuickCheck is of marginal interest at best. Next time I care what the value is I'll reenable it, but until then we can leave it off.

@ndmitchell ndmitchell closed this May 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment