Re: [julia] Bump MAX_TYPE_DEPTH again (#13561)
Tim Holy
Tim Holy
OOM killer strikes again. Restarting jobs.
John Myles White
We should really consider tunning this with settings from 3 to 6 on the perf benchmarks at some point.
Tim Holy
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 tlycken/Interpolations.jl#79 it's a factor of ~100x.
Simon Kornblith
It might be interesting to be able to print the situations where we hit the hard-coded limits here.
Tony Kelman
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.
Tim Holy
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?
Tony Kelman
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?
Tim Holy
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 Fischer
That change should be a no-op in the absence of address sanitizer.
Yichao Yu
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.
Tim Holy
Since it seems this is an unlikely culprit, I'll merge in the next day unless there are objections.
Tim Holy
Merged #13561.
Tim Holy
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.