Attempt to speed solving when test is enable projectwide #7490
Conversation
|
I need to look more closely at the original issue this PR is fixing, but I had some initial comments. I believe that the reason that constraints do not reorder nodes is that every time a constraint would reject a stanza or version choice, the solver should be able to backtrack in the next step, so reordering nodes shouldn't be significantly faster. Additionally, using the original node order gives better error messages, because rejecting the disallowed version/stanza choice creates a solver log message describing why the choice was rejected (including the source of any constraint). From the log message in the issue ("rejecting: C:B:exe.A:!test (dependencies not linked: ..."), it looks like the performance issue is caused by the solver's preference for reusing the same instance of A for both C's indirect build tool dependency and the requirement to build all local packages. Those are different solver goals (A vs C:B:exe.A), so they are not required to make the same choices for enabling tests. The two goals are only required to make the same choices for enabling tests if they are linked, and the solver should be able to backtrack further to unlink them if necessary. I'm not familiar with why ProjectPlanning.hs adds a stanza preference only when there is no constraint. I would have expected cabal to add the same preferences to all local packages. |
|
Thanks for the comments! Its not clear to me if backtracking due to test stanzas being enabled or not ever was a performance issue. @michaelpj raised it as one, but that was simply one explanation possible for why on a large project enabling tests reduces solver speed. It could well be that enabling tests just makes the solvers life much harder -- i.e. it introduces a lot more dependencies with a lot more constraints. I've noticed, for example, that when people bump constraints on a package they often don't bump them on the test stanza as frequently/eagerly since it matters less to end users (I've been lazy and done this myself!). In such situations, enabling test stanzas means that the constraint problem becomes much trickier and more computationally expensive to solve. As is, I don't think the repro case given suffices to sort this out -- it lets us see that backtracking occurs, but since the backtracking is (as your comment describes) extremely local, it isn't clear to me that this is the root cause of any observed slowdown, and the repro case doesn't let us observe the slowdown itself. |
|
Yeah, solving for the test dependencies could be very time consuming, though I also wonder if the performance problem only appears when there are more packages in the chain of dependencies related to the build tool. I can see one reason for only applying test/bench preferences when there are no constraints. It could lead to better error messages, because the solver log shows when a constraint rules out a choice, but it doesn't show when preferences prevent a choice from being considered. I think that it would be better to apply the same preferences to all local packages for consistency, though. |
|
I tried this branch and got the following failure: |
|
@newhoggy: I tried to reproduce using cabal's master branch (to verify that the regression is from this PR), but failed due to lack of Would it be a problem for you to run the test above with cabal master branch? Another speedup by @gbaz is already merged into master (a few related expensive assertions disabled for non-CI builds), so you'd also have a chance to measure the impact, if you have any cabal benchmarks ready. |
|
@newhoggy fixed the bug well enough so you can test this branch with --tests-enabled. This branch still fails on a related bug with --tests-disabled. I believe this is because the following is incorrect: In particular In any case, even without it fixed, please give this branch a try vs plain master (I just rebased to master too) and see if you get any improvement? |
|
@gbaz your new commit works for me. Is there a way to force cabal to solve again rather than use the cached solution? And is there a way to measure the solving time without all the other things that cabal does? |
|
To help with the testing, the following Contrary to documentation, Note this is not how the project is supposed to be built (specifically we use a As for building the project: |
To clear the cached solution, deleting the Passing |
|
Before: After: This is a big improvement, but there is still quite a way to the point of parity with deleting build-tool-depends declarations that were already in the build-plan: The last run is based on this diff: |
|
Wow, I'm really pleased this commit made a difference after all -- I was worried about if it would! As discussed in the other thread, I think there may still be more to be done. I'll try to fix the localEnabledStanzas issue so that this can be merged, as a start. |
|
Found and fixed the bug, which was naturally in my own code, and not elsewhere. Given that this seems to bring about a performance improvement in large use cases, and that it doesn't hurt correctness (and makes logs more readable) I'd like some reviews so we can proceed to a merge. |
| @@ -1024,13 +1024,13 @@ planPackages verbosity comp platform solver SolverSettings{..} | |||
| | (pc, src) <- solverSettingConstraints ] | |||
|
|
|||
| . addPreferences | |||
| -- enable stanza preference where the user did not specify | |||
| -- enable stanza preference unilaterally, even when the user asked to enable as well, to help hint the solver. | |||
There was a problem hiding this comment.
to make it clearer (future readers won't have the previous version of this code as context):
-- enable stanza preference unilaterally, regardless if the user asked
-- accordingly or expressed no preference, to help hint the solver
|
I'm not completely sure I understand how this PR is supposed to change the behavior of the solver. I think there may be some inaccuracies in the comments. The comment in Preference.hs says that the preferences are applied to all uses of a package because the stanza choices must be compatible. However, the solver can make different choices for different goals involving the same package (with different qualifiers). For example, it could enable tests for package x when it is used as a build-depends dependency of a build target but disable tests for x when it is used as a setup dependency. Making different stanza choices is less common than making different choices for the version of a package, though, since users usually only enable tests for local packages, which can only have once instance (due to #6459). The comment in ProjectPlanning.hs says that applying stanza preferences to local packages that also have tests enabled hints the solver. These preferences should have almost no effect on solver performance, because the constraint would already cause the solver to backtrack immediately. |
|
Experimentally, I guess we could disable one half of this PR and benchmark to see from which part the significnt benefit comes. |
The comment could be improved. That said, my understanding of the situation is as follows. The solver can make different choices involving the same package. However, stanza preferences are only applied to local packages, and since we have a constraint that we can't make use of two installs of the same version of a package, and since a local package is constrained to be at a single version, as you note, then for the case of stanza preferences in particular, we really can't make different choices involving the same package.
Well that's what we thought -- but the example of The other approach could be to try to take a topological sort of the build dependency order of local packages and apply that to the tree before solving. (And arguably that's better for things like flags, etc, which can be more complex), but since that interacts with version solving, and its a full other pass, that could be a trickier heuristic to write... |
|
@Mikolaj let me try to explain the two stupid bugs I introduced and then fixed to help your confidence level. The first bug (fixed by 624b8dc) was that it turned on stanzas always, even when they were explicitly disabled, and even when the package was not local. (I figured the constraints would suffice to override the preferences, but they did not). The second bug (fixed by a5d0445) was a fix to my previous fix -- which still turned on Both these cases didn't hit errors in the solver, but only triggered sanity checks in the build-plan generation, and to trigger the latter required a cabal.project file which has explicit test disabled flag for specific packages. |
|
@gbaz Thanks for the explanation. It looks like the PR improves performance because it sets stanza preferences on local packages when they are used as build tool and setup dependencies, which do not have stanza constraints applied. I still think that the best way to solve #7472 is to implement #6459. That feature would allow the solver to perform well regardless of the stanza constraints and preferences that it receives as inputs. Then other parts of cabal could pass in the constraints and preferences to produce the desired behavior, without being concerned with performance. I also think that #6459 could be difficult to implement, so it may be necessary to first fix the performance issue in a simpler, more targeted way. I think that a more direct way to prevent the backtracking in #7472 is to apply stanza constraints (not preferences) to the build tool and setup dependencies in ProjectPlanning.hs. Controlling the qualifiers that constraints apply to is already implemented, so it wouldn't require any changes to the solver. I think that would only require changing |
|
I also think that applying stanza preferences as I do in this PR makes sense as well. Shall I amend this PR to apply stanza constraints at all levels too? I think flag constraints are maybe a bit trickier, since they can apply to packages that are not local as well. Perhaps we should also apply those at all levels but only in the case when the package is local? This speeds up cases like here, without causing any semantic changes that would harm the solver otherwise... |
Yes, I think that also modifying the stanza preferences is fine. Then stanza preferences and constraints will be applied consistently. It would also lead to more predictable install plans when tests are not explicitly enabled or disabled.
Are you referring to the flags in #7472 (comment)? I think that is a different issue, because it is more about the semantics of cabal.project, especially for the case of non-local packages. For local packages, it raises the question of whether we should force each local package to only be built once, even after #6459 is fixed. |
|
On reflection, I'm not going to add constraints, just leave it at preferences. That way when #6459 is fixed, there's no changes that need to be reverted here. Added a patch to touchup the comments for clarity, and will merge. |
|
Thanks, looking forward to trying this out! |
Resolves #7472
The good news is that these two changes should be simple and easy to understand, and resolve the issue presented. They also should be (to the extent I understand the solver) unilaterally safe, and almost certainly improvements.
The bad news is that it is not clear to me how much performance win we get from this -- in the small test case, the affect on solving only seemed to cut out some very small local backjumps, nothing that could account for geometric behavior.
The key notion here is "stanza preferences" -- these only affect ordering of attempts to try stanzas off vs. on, so switching preferences should not hurt correctness, only modify the order in which certain things are tried.
The first change takes account of stanza preferences not only for top level packages, but also when they are used as dependencies of other packages in the project. It makes sense to do this, since we need to match how we pick stanzas throughout.
The second change modifies the "default preferences" to always suggest trying first with all stanzas enabled. Before, it had the counterintuitive and probably conceptually wrong behavior of trying first with all stanzas enabled except those the user specifically constrained to be enabled. Because a backjump would occur in such a case anyway, there was no correctness problem, but there was certainly a small inefficiency in exploring the tree.
So both changes appear to be simple, understandable, and obviously more correct. However, again, I'm not sure if the effect on solving will be noticeable in the large projects that prompted this ticket.