-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
rustc runs out of memory when compiling trait heavy code #43613
Comments
EDIT I may have been mistaken about the example from #32062. While compilation takes far longer than one would expect, that has likely always been the case as well (still a potential issue but perhaps not a reproduction for the one I encountered). The example added for #32062 reproduces as long as few more calls to chain gets added
|
@Marwes did this compile previously? Just wondering if we can tag it as a regression! |
cc @arielb1 for potential regression tracking |
@alexcrichton I am afraid the the example from #32062 would fail to compile before as well so it is likely unrelated. https://github.com/Marwes/combine/tree/ice_allocator_exhausted did not compile before due to #43132 so this is probably not a regression. |
A tangentially related comment: on nightly, no error is printed when the compiler fails this way beyond |
Ah unfortunately this is a beta regression. Compiling this source I get:
|
cc @rust-lang/compiler for a new beta regression |
triage: P-high |
We backed out quite a few PRs for beta .3. Could you test again after that? |
Looks like it helped a little but not a huge amount, the beta timing decreased from ~7 to ~5s It looks like, however, that it may be constant overhead. Commenting out a few more Should this be closed? |
I am reclassifying as a regression against nightly. Still relevant. |
Ok, switching milestone in that case as well. Thanks! |
1.21 is now in beta |
We should retest -- @arielb1, have your various improvements landed? I'm assigning to us both to try and investigate. |
OK, so, using nightly, I am able to build the original
|
OK, I did some digging. There is a massive spike (when compiling |
Relevant backtrace from memcache report:
|
@nikomatsakis is it reasonable to assume that this and #43789 are really the same issue? |
I did more investigation. In particular, I tried an experimental idea to reduce the size of the cache as soon as we can. This did reduce, the peak but not by much. Here is the "ideal" memory profile, based on the old caching policy around obligations:
This is the profile of the current master:
Notice the several peaks. Each of those are an "episode" the cache in use. This is the profile with my patch, which clears the cache obligations after a cache hit in which we observe that there are no unbound type variables in the projection result:
You can see it's improved, and the peak is lower in an absolute sense, but there is still a clear peak. I'm done for the day but will poke at this later on. |
OK, I tried a more aggressive pruning pass, and that seems to have worked out pretty well:
We're still using more memory, but it's a much more mild spike -- and execution time is about the same. Now I just have to make sure that I didn't break anything else along the way. =) |
Keep **all** the obligations for every projection is wasteful of memory and compilation time. We only really care about those subobligations that may inform the result of the projection (i.e., may help to resolve any inference variables that appear within). Therefore, we can clear the subobligations from the cache that don't potentially affect the result of the projection. On every cache hit, we also take the opportunity to check if the type variables have been resolved *yet* and, if so, clear out the pending obligations. Fixes rust-lang#43613
PR opened (#44269) |
The version in the PR prunes mildly less aggressively -- I think my initial patches were incorrect -- but does remember to call
|
…ns, r=arielb1 limit and clear cache obligations opportunistically Keeping **all** the obligations for every projection is wasteful of memory and compilation time. We only really care about those subobligations that may inform the result of the projection (i.e., may help to resolve any inference variables that appear within). Therefore, we can clear the subobligations from the cache that don't potentially affect the result of the projection. On every cache hit, we also take the opportunity to check if the type variables have been resolved *yet* and, if so, clear out the pending obligations. Fixes #43613. r? @arielb1 NB -- not sure how to test for this. Probably we should add the #43613 test case to perf.
Keep **all** the obligations for every projection is wasteful of memory and compilation time. We only really care about those subobligations that may inform the result of the projection (i.e., may help to resolve any inference variables that appear within). Therefore, we can clear the subobligations from the cache that don't potentially affect the result of the projection. On every cache hit, we also take the opportunity to check if the type variables have been resolved *yet* and, if so, clear out the pending obligations. Fixes rust-lang#43613
We want to retain obligations that *contain* inference variables, not obligations that *don't contain* them, in order to fix rust-lang#43132. Because of surrounding changes to inference, the ICE doesn't occur in its original case, but I believe it could still be made to occur on master. Maybe I should try to write a new test case? Certainly not right now (I'm mainly trying to get us a beta that we can ship) but maybe before we land this PR on nightly? This seems to cause a 10% performance regression in my imprecise attempt to benchmark item-body checking for rust-lang#43613, but it's better to be slow and right than fast and wrong. If we want to recover that, I think we can change the constrained-type-parameter code to actually give a list of projections that are important for resolving inference variables and filter everything else out.
We want to retain obligations that *contain* inference variables, not obligations that *don't contain* them, in order to fix rust-lang#43132. Because of surrounding changes to inference, the ICE doesn't occur in its original case, but I believe it could still be made to occur on master. Maybe I should try to write a new test case? Certainly not right now (I'm mainly trying to get us a beta that we can ship) but maybe before we land this PR on nightly? This seems to cause a 10% performance regression in my imprecise attempt to benchmark item-body checking for rust-lang#43613, but it's better to be slow and right than fast and wrong. If we want to recover that, I think we can change the constrained-type-parameter code to actually give a list of projections that are important for resolving inference variables and filter everything else out.
fix logic error in #44269's `prune_cache_value_obligations` We want to retain obligations that *contain* inference variables, not obligations that *don't contain* them, in order to fix #43132. Because of surrounding changes to inference, the ICE doesn't occur in its original case, but I believe it could still be made to occur on master. Maybe I should try to write a new test case? Certainly not right now (I'm mainly trying to get us a beta that we can ship) but maybe before we land this PR on nightly? This seems to cause a 10% performance regression in my imprecise attempt to benchmark item-body checking for #43613, but it's better to be slow and right than fast and wrong. If we want to recover that, I think we can change the constrained-type-parameter code to actually give a list of projections that are important for resolving inference variables and filter everything else out.
Been working on an update to combine and ran into #43132. Since the fix has been merged I updated using rustup but now rustc runs out of memory when compiling instead.
This branch reproduces the issue https://github.com/Marwes/combine/tree/ice_allocator_exhausted. I haven't bothered to reduce to a minimal production given that it is likely related to the slow down mentioned in #43132 and removing code will just make it compile and make the problem less pronounced (possibly the minimal example here reproduces the same problem as well https://github.com/rust-lang/rust/pull/32062/files#diff-6a2bc45424fb5e8e0f7eb99f2e05f9cf).
The text was updated successfully, but these errors were encountered: