close
Skip to content

Count constraint-solving memory more precisely, rdar://29684330#7088

Merged
swift-ci merged 1 commit intoswiftlang:masterfrom
graydon:rdar-29684330-count-constraint-solving-memory-more-precisely
Feb 6, 2017
Merged

Count constraint-solving memory more precisely, rdar://29684330#7088
swift-ci merged 1 commit intoswiftlang:masterfrom
graydon:rdar-29684330-count-constraint-solving-memory-more-precisely

Conversation

@graydon
Copy link
Copy Markdown
Contributor

@graydon graydon commented Jan 27, 2017

The type constraint solver guides some of its thinking about when to give up on a
sub-problem by measuring its use of memory. Unfortunately it wasn't measuring that
number very accurately, which meant that it kept going far longer than it should have
on certain cases (and chewed up far too much real memory in the process).

This fixes two points of mis-measurement: the solver-arena's bump allocator (which isn't
actually counted in the existing getTotalMemory call) and, more critically, the cost of
the vector of solutions being accumulated itself.

rdar://29684330

@graydon
Copy link
Copy Markdown
Contributor Author

graydon commented Jan 27, 2017

The second bit of accounting here is perhaps debatable, and I'd appreciate conceptual double-checking by someone who has thought longer about this. It seems to me that it's more truthy to consider this (very large) temporary vector-of-objects it's building as "part of" the solver's memory use, but an argument could be made the other way.

In any case it seems to behave much better with this patch!

@graydon graydon requested review from DougGregor and rudkx January 27, 2017 09:46
@rudkx
Copy link
Copy Markdown
Contributor

rudkx commented Jan 27, 2017

I added a new metric that is enabled when !Swift3, and I think we need to look at making sure these issues are fixed on that path as well. A couple things I noticed when I was debugging one of these cases (but didn't have a chance to really look into in detail):

  • The value TypeCounter in ConstraintSystem was really large despite not having that many type variables, so there might be something wrong with my new metric, or something wrong with how TypeCounter is getting incremented.
  • CountScopes was not increasing despite the fact that we were allocating a bunch of memory in gatherConstraints(), which leads me to believe that perhaps we were just spinning somewhere rather than actively making progress in the solver. If that's the case the new metric might need to be tweaked for these cases as well.

Can you take a look at that path and see what's happening?

Copy link
Copy Markdown
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

This generally looks good, but aren't we still missing the distinction between "failed because I found no solutions" and "failed because I hit a threshold"?

@graydon graydon force-pushed the rdar-29684330-count-constraint-solving-memory-more-precisely branch from b920064 to f2b6eff Compare January 31, 2017 04:36
@graydon
Copy link
Copy Markdown
Contributor Author

graydon commented Jan 31, 2017

Updated with memory high-water mark to at least make the memory-pressure-based judgment sticky; this means it now reports "too complex" properly in both swift 3 and 4 modes.

Re: feedback from @DougGregor, yes, it seems that the existing system doesn't differentiate those cases in the return codes, but it does check the "too-complex-ness" of an unsolved constraint system during salvage, when revisiting it in CSDiag. I just needed to make that property sticky so that it doesn't get judged too-complex in CSSolver and not-too-complex in CSDiag.

Will try looking into the questions @rudkx asks above wrt. the non-memory signals. Initial scan of -debug-constraint logs looks like we're just "not quite hitting the thresholds"; around 200 type variables and only a few thousand scopes. Lowering the scope threshold to a few thousand means it errors out, for example. The problem is that it's allocating gigabytes of memory along the way, accumulating solutions. I'll try to characterize the workload in the associated testcase a little better but I'm still rather unclear on how the solver works. Learning while I go.

Meanwhile, running tests!

@graydon
Copy link
Copy Markdown
Contributor Author

graydon commented Jan 31, 2017

@swift-ci please smoke test

@graydon graydon force-pushed the rdar-29684330-count-constraint-solving-memory-more-precisely branch from f2b6eff to 5938d84 Compare February 4, 2017 01:50
@graydon graydon changed the title [Don't Merge Yet] Count constraint-solving memory more precisely, rdar://29684330 Count constraint-solving memory more precisely, rdar://29684330 Feb 6, 2017
@graydon graydon force-pushed the rdar-29684330-count-constraint-solving-memory-more-precisely branch from 5938d84 to c15c2c2 Compare February 6, 2017 19:00
@graydon graydon requested a review from DougGregor February 6, 2017 19:05
@graydon
Copy link
Copy Markdown
Contributor Author

graydon commented Feb 6, 2017

@swift-ci please test

@swift-ci
Copy link
Copy Markdown
Contributor

swift-ci commented Feb 6, 2017

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - b92006467751cceecfe565c603743b6b81d915aa
Test requested by - @graydon

@swift-ci
Copy link
Copy Markdown
Contributor

swift-ci commented Feb 6, 2017

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - b92006467751cceecfe565c603743b6b81d915aa
Test requested by - @graydon

@graydon graydon force-pushed the rdar-29684330-count-constraint-solving-memory-more-precisely branch from c15c2c2 to 9cb1c52 Compare February 6, 2017 21:06
Copy link
Copy Markdown
Contributor

@rudkx rudkx left a comment

Choose a reason for hiding this comment

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

LGTM.

Thanks for looking into this!

@graydon
Copy link
Copy Markdown
Contributor Author

graydon commented Feb 6, 2017

@swift-ci please smoke test and merge

1 similar comment
@graydon
Copy link
Copy Markdown
Contributor Author

graydon commented Feb 6, 2017

@swift-ci please smoke test and merge

@swift-ci swift-ci merged commit 014abc3 into swiftlang:master Feb 6, 2017
@graydon graydon deleted the rdar-29684330-count-constraint-solving-memory-more-precisely branch September 13, 2017 16:42
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.

4 participants