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

Bump MAX_TYPE_DEPTH again #13561

Merged
merged 1 commit into from
Oct 17, 2015
Merged

Bump MAX_TYPE_DEPTH again #13561

merged 1 commit into from
Oct 17, 2015

Conversation

timholy
Copy link
Member

@timholy timholy commented Oct 12, 2015

This is needed to fix the performance problem noted in JuliaMath/Interpolations.jl#79.

This seems to be within the noise in terms of effect on compilation time; I've posted a gist for anyone who wants to see individual test times. (Look first at the file called "Test times.") EDIT: Total change was 2.5%. It looked like it was having an effect on the linalg times, but when I re-ran the branch on just the linalg tests, that concern evaporated. So I suspect the differences were mostly due to noise & perhaps the fact that this server is running other stuff.

I limited to 4 cores because otherwise the total time is dominated by waiting for the SubArray tests to finish.

@timholy
Copy link
Member Author

timholy commented Oct 12, 2015

@JuliaBackports

@timholy
Copy link
Member Author

timholy commented Oct 12, 2015

OOM killer strikes again. Restarting jobs.

@johnmyleswhite
Copy link
Member

We should really consider tunning this with settings from 3 to 6 on the perf benchmarks at some point.

@timholy
Copy link
Member Author

timholy commented Oct 12, 2015

AFAICT there's nothing to tune: the range from 4 to 7 (at least) has no impact on the speed at which the tests run. Much more important is whether people can have inference run successfully on their code (which of course does have a performance impact; in the case of JuliaMath/Interpolations.jl#79 it's a factor of ~100x).

@simonster
Copy link
Member

It might be interesting to be able to print the situations where we hit the hard-coded limits here.

@tkelman
Copy link
Contributor

tkelman commented Oct 12, 2015

I still need to write down a more formal backporting policy somewhere, but just fyi if there's an issue or PR associated with a commit to backport, I find the label much easier to track than mentions of @juliabackports - I only use the latter for commits to master that don't have any linked issues or PR's.

@timholy
Copy link
Member Author

timholy commented Oct 12, 2015

Thanks for explaining, @tkelman.

This has OOMed 3 times in a row. Is that a cause for concern, or are we seeing similar rates of failure elsewhere. If so, should we look into what increased the memory pressure?

@tkelman
Copy link
Contributor

tkelman commented Oct 12, 2015

We are seeing very frequent OOM failures elsewhere, yes we should look into it (I suspect the line number node changes?), but this PR might also be making the problem worse?

@timholy
Copy link
Member Author

timholy commented Oct 12, 2015

From the history of builds, https://travis-ci.org/JuliaLang/julia/builds, it seems that a possible candidate is af71b86. CC @Keno. Would it be surprising that something affecting dlload would cause this?

@Keno
Copy link
Member

Keno commented Oct 12, 2015

That change should be a no-op in the absence of address sanitizer.

@yuyichao
Copy link
Contributor

I suspect the line number node changes

That's also what I would suspect. The typed AST is a lot larger now because of all the line number node (many of which might be unecessary). Given that a significant fraction of the (persistent) memory is the code we generate, Increasing the size of the AST we store would probably increase the total memory by a lot as well.

@timholy
Copy link
Member Author

timholy commented Oct 14, 2015

Since it seems this is an unlikely culprit, I'll merge in the next day unless there are objections.

timholy added a commit that referenced this pull request Oct 17, 2015
@timholy timholy merged commit b05c30c into master Oct 17, 2015
@timholy timholy deleted the teh/max_type_depth branch October 17, 2015 12:27
@timholy
Copy link
Member Author

timholy commented Oct 17, 2015

It will be interesting to see if bumping this to 7 holds us for very long. As our ecosystem grows, it's getting ridiculously easy to hit the current limits (I seem to hit this particular issue about once per day). For example, while I initially discovered this with Interpolations, moments ago I just triggered this by combining ForwardDiff with a SubArray. These are all reasonable things to want to do, and the limits in inference trigger errors (e.g., no method zero(::Type{Any})) or kill performance. (Clearly I just need to consistently apply various patches to all the machines I work on, but I often seem to leave one out and then have ~20 minutes of needless head-scratching.)

MAX_TYPE_DEPTH 7 seems to work for me for now, but I wonder if the next bump should be to 20 or so.

timholy added a commit that referenced this pull request Oct 31, 2015
(cherry picked from commit 658a0a8)
ref #13561
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.

6 participants