Bail out of reducing depency trees on huge dependency conflict sets #6145
Conversation
This is a simple fix for rubygems#6114, in which sometimes conflict sets get so huge that the current reducing code (which is based on Array#combination) breaks down. By simply bailing out when the conflict set is bigger than a certain size, we'll get a result that is similar to what happened in earlier bundler versions but skip a ton of CPU cycles and memory allocations. I've chosen the limit rather unscientifically by playing around with the result set sizes. 15 seems to be a good compromise but really anything larger than 10 should work (and with work I mean "should not usually not have to be invoked").
|
Thanks for opening a pull request and helping make Bundler better! Someone from the Bundler team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality. We use Travis CI to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of Travis CI in the PR status window below. If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #bundler channel on Slack. For more information about contributing to the Bundler project feel free to review our CONTRIBUTING guide |
|
@segiddins Thanks for the approval. The test failure kind of baffles me, though. I don't really understand the failure message and also this seems to be a failing test against Ruby 1.8.7 - I thought bundler 2.0 is not supposed to run on anyithing below Ruby 2? (Mind this could be me just not comprehending your test setup which is somewhat complex, understandably so) |
|
@segiddins After taking another look and seeing that your own PR #6142 fails with the exact same problem: Could this be something related to the 1.6 release? Anyway, not wanting to fix something that isn't really broken, I'll await your comment. (One thing I did notice is that the "bundle pristine" call does not use the full path to the bundler executable as the other calls prior seem to use, could this be the issue?) |
|
@bundlerbot r+ |
|
📌 Commit c50e30c has been approved by |
…iddins Bail out of reducing depency trees on huge dependency conflict sets ### What was the end-user problem that led to this PR? The problem is described in #6114 - An unusually large set of conflicting dependencies leads to bundler get stuck and lead to very unhealthy amounts of memory and CPU time be consumed. ### What was your diagnosis of the problem? This happens because because a too large array is fed into Array#combination in the "reduce_trees" lambda in `version_conflict_message`. ### What is your fix for the problem, implemented in this PR? By simply bailing out when the conflict set is bigger than a certain size, we'll get a result that is similar to what happened in earlier bundler versions but skip a ton of CPU cycles and memory allocations. I've chosen the limit rather unscientifically by playing around with the result set sizes. 15 seems to be a good compromise but really anything larger than 10 should work (and with work I mean "should not usually not have to be invoked"). ### Why did you choose this fix out of the possible options? Reducing the conflict trees has been a rather new addition to bundler, so defaulting back to the old behavior of not reducing the trees seems like an OK option. It may be possible to also optimize the reduction algorithm, but since this is very much an edge case that only happens when using Bundler slightly out of the normal procedures, I think a simple bail out is sufficient.
|
☀️ Test successful - status-travis |
…iddins Bail out of reducing depency trees on huge dependency conflict sets ### What was the end-user problem that led to this PR? The problem is described in #6114 - An unusually large set of conflicting dependencies leads to bundler get stuck and lead to very unhealthy amounts of memory and CPU time be consumed. ### What was your diagnosis of the problem? This happens because because a too large array is fed into Array#combination in the "reduce_trees" lambda in `version_conflict_message`. ### What is your fix for the problem, implemented in this PR? By simply bailing out when the conflict set is bigger than a certain size, we'll get a result that is similar to what happened in earlier bundler versions but skip a ton of CPU cycles and memory allocations. I've chosen the limit rather unscientifically by playing around with the result set sizes. 15 seems to be a good compromise but really anything larger than 10 should work (and with work I mean "should not usually not have to be invoked"). ### Why did you choose this fix out of the possible options? Reducing the conflict trees has been a rather new addition to bundler, so defaulting back to the old behavior of not reducing the trees seems like an OK option. It may be possible to also optimize the reduction algorithm, but since this is very much an edge case that only happens when using Bundler slightly out of the normal procedures, I think a simple bail out is sufficient. (cherry picked from commit fc341ed)
What was the end-user problem that led to this PR?
The problem is described in #6114 - An unusually large set of conflicting dependencies leads to bundler get stuck and lead to very unhealthy amounts of memory and CPU time be consumed.
What was your diagnosis of the problem?
This happens because because a too large array is fed into Array#combination in the "reduce_trees" lambda in
version_conflict_message.What is your fix for the problem, implemented in this PR?
By simply bailing out when the conflict set is bigger than a certain size,
we'll get a result that is similar to what happened in earlier bundler versions
but skip a ton of CPU cycles and memory allocations.
I've chosen the limit rather unscientifically by playing around with the result
set sizes. 15 seems to be a good compromise but really anything larger than 10
should work (and with work I mean "should not usually not have to be invoked").
Why did you choose this fix out of the possible options?
Reducing the conflict trees has been a rather new addition to bundler, so defaulting back to the old behavior of not reducing the trees seems like an OK option. It may be possible to also optimize the reduction algorithm, but since this is very much an edge case that only happens when using Bundler slightly out of the normal procedures, I think a simple bail out is sufficient.