Fix hls-graph: phantom dependencies invoke in branching deps (resolve #3423)

[soulomoon]

phantom depencies is invoke becase dependencies have preconditions in rules, see #3423. This pr is intend to fix that.
This might also fix some of the flaky tests.

Introduction a new ResultDepsTree to ResultDeps which contains a treelike minimap of dependencies for a key.
It provides the branching semantic when we reflesh a key. Key in depsTreeNodeCond is responsible for computing the branching condition. It equipped a continuation depsTreeContinuation, to decided what dependencies are needed after that.
Pros: we have correct semantic for the rules with branching dependencies.
cons:It requires us to manually annotate the rule by providing the ResultDepsTree.<\del>

In favor of @wz1000 appoach of running deps linearly.
It modify the deps result from KeySet to [KeySet] to make sure the result is sorted

we initialy thought it would have performance impact on the build system. But it turns out instead of performance lost, we actaully have performance gain since it avoid building the phantom depencies.

Overall things have been done:

1. Fix up hls-graph phantom depencies issue by reflesh linear deps in a linear manner.
2. Fix the bench for macos and added semantic tokens bench mark.
3. add test to hls-graph to ensure phantom depencies would not be invoke.

Result:
Now no more phantom dependencies would be invoked in hls-graph, gaining correctness, less runtime and less mem usage at the some time.

[soulomoon]

First bench mark

| version  | configuration | name                           | success | samples | startup | setup | userT  | delayedT | 1stBuildT | avgPerRespT | totalT | rulesBuilt | rulesChanged | rulesVisited | rulesTotal | ruleEdges | ghcRebuilds | maxResidency | allocatedBytes |
| -------- | ------------- | ------------------------------ | ------- | ------- | ------- | ----- | ------ | -------- | --------- | ----------- | ------ | ---------- | ------------ | ------------ | ---------- | --------- | ----------- | ------------ | -------------- |
| upstream | All           | edit-header                    | True    | 50      | 2.00    | 0.00  | 30.51  | 0.05     | 2.84      | 0.28        | 30.57  | 14         | 12           | 1363         | 5910       | 31261     | 51          | 290MB        | 136737MB       |
| HEAD     | All           | edit-header                    | True    | 50      | 1.97    | 0.00  | 36.75  | 0.07     | 2.81      | 0.35        | 36.82  | 17         | 14           | 1367         | 5910       | 31261     | 51          | 298MB        | 160490MB       |
| upstream | All           | edit                           | True    | 50      | 2.39    | 0.00  | 30.74  | 0.03     | 2.82      | 0.28        | 30.77  | 14         | 12           | 1363         | 5909       | 31255     | 51          | 296MB        | 134882MB       |
| HEAD     | All           | edit                           | True    | 50      | 2.04    | 0.00  | 35.39  | 0.02     | 2.81      | 0.33        | 35.42  | 17         | 14           | 1367         | 5910       | 31261     | 51          | 297MB        | 156089MB       |
| upstream | All           | hover                          | True    | 50      | 2.02    | 0.00  | 2.85   | 0.01     | 2.81      | 0.00        | 2.86   | 5094       | 5093         | 5094         | 5912       | 31267     | 2           | 164MB        | 15418MB        |
| HEAD     | All           | hover                          | True    | 50      | 1.85    | 0.00  | 2.82   | 0.01     | 2.78      | 0.00        | 2.83   | 5126       | 5125         | 5126         | 5912       | 31267     | 2           | 127MB        | 15332MB        |
| upstream | All           | semanticTokens                 | True    | 50      | 1.97    | 0.00  | 66.98  | 1.28     | 3.65      | 0.65        | 68.27  | 12         | 10           | 1364         | 5914       | 31271     | 52          | 298MB        | 289880MB       |
| HEAD     | All           | semanticTokens                 | True    | 50      | 1.94    | 0.00  | 67.98  | 1.11     | 3.43      | 0.66        | 69.10  | 12         | 10           | 1364         | 5914       | 31271     | 52          | 300MB        | 301805MB       |
| upstream | All           | hover after edit               | True    | 50      | 1.92    | 0.00  | 3.08   | 30.96    | 2.94      | 0.00        | 34.04  | 21         | 18           | 1371         | 5912       | 31267     | 51          | 305MB        | 146999MB       |
| HEAD     | All           | hover after edit               | True    | 50      | 2.01    | 0.00  | 3.06   | 34.95    | 2.93      | 0.00        | 38.01  | 21         | 18           | 1371         | 5912       | 31267     | 51          | 308MB        | 162980MB       |
| upstream | All           | getDefinition                  | False   | 50      | 1.99    | 0.00  | 0.00   | 0.00     | 0.00      | 0.00        | 2.87   | 5125       | 5124         | 5125         | 5907       | 31262     | 2           | 158MB        | 15198MB        |
| HEAD     | All           | getDefinition                  | False   | 50      | 1.95    | 0.00  | 0.00   | 0.00     | 0.00      | 0.00        | 2.68   | 5122       | 5121         | 5122         | 5907       | 31262     | 2           | 189MB        | 15108MB        |
| upstream | All           | getDefinition after edit       | False   | 50      | 2.17    | 0.00  | 0.00   | 0.00     | 0.00      | 0.00        | 3.30   | 5127       | 5125         | 5127         | 5907       | 31262     | 2           | 134MB        | 15222MB        |
| HEAD     | All           | getDefinition after edit       | False   | 50      | 1.87    | 0.00  | 0.00   | 0.00     | 0.00      | 0.00        | 2.87   | 5128       | 5126         | 5128         | 5907       | 31262     | 2           | 164MB        | 15264MB        |
| upstream | All           | completions                    | True    | 50      | 1.98    | 0.00  | 5.01   | 0.01     | 3.00      | 0.02        | 5.03   | 5124       | 5123         | 5124         | 5914       | 31265     | 2           | 244MB        | 20874MB        |
| HEAD     | All           | completions                    | True    | 50      | 1.92    | 0.00  | 4.81   | 0.01     | 3.06      | 0.02        | 4.82   | 5124       | 5123         | 5124         | 5914       | 31265     | 2           | 221MB        | 20876MB        |
| upstream | All           | completions after edit         | True    | 50      | 1.98    | 0.00  | 12.07  | 22.16    | 3.16      | 0.09        | 34.24  | 23         | 20           | 1373         | 5914       | 31265     | 51          | 314MB        | 146901MB       |
| HEAD     | All           | completions after edit         | True    | 50      | 1.90    | 0.00  | 12.24  | 25.71    | 3.16      | 0.09        | 37.96  | 23         | 20           | 1373         | 5914       | 31265     | 51          | 324MB        | 164263MB       |
| upstream | All           | code actions                   | True    | 50      | 1.98    | 0.00  | 4.62   | 0.02     | 3.07      | 0.02        | 4.65   | 5184       | 5183         | 5184         | 5965       | 31638     | 2           | 213MB        | 23574MB        |
| HEAD     | All           | code actions                   | True    | 50      | 1.89    | 0.00  | 4.37   | 0.02     | 2.72      | 0.02        | 4.40   | 5176       | 5175         | 5176         | 5965       | 31638     | 2           | 209MB        | 23598MB        |
| upstream | All           | code actions after edit        | True    | 50      | 1.97    | 0.00  | 33.59  | 0.02     | 3.10      | 0.31        | 33.62  | 19         | 17           | 1370         | 5965       | 31638     | 51          | 290MB        | 144401MB       |
| HEAD     | All           | code actions after edit        | True    | 50      | 2.00    | 0.00  | 38.98  | 0.03     | 3.01      | 0.37        | 39.02  | 18         | 16           | 1369         | 5965       | 31638     | 51          | 317MB        | 164821MB       |
| upstream | All           | code actions after cradle edit | True    | 50      | 2.13    | 0.00  | 210.18 | 0.08     | 5.25      | 2.09        | 210.27 | 1299       | 803          | 2538         | 5965       | 31393     | 100         | 506MB        | 368322MB       |
| HEAD     | All           | code actions after cradle edit | True    | 50      | 1.86    | 0.00  | 159.57 | 0.17     | 4.52      | 1.58        | 159.74 | 1299       | 803          | 2538         | 5965       | 31393     | 100         | 505MB        | 308065MB       |
| upstream | All           | documentSymbols after edit     | True    | 50      | 1.97    | 0.00  | 5.73   | 3.37     | 3.00      | 0.06        | 9.10   | 14         | 11           | 1362         | 5905       | 31249     | 3           | 233MB        | 44703MB        |
| HEAD     | All           | documentSymbols after edit     | True    | 50      | 1.87    | 0.00  | 6.00   | 2.88     | 2.78      | 0.06        | 8.89   | 13         | 11           | 1360         | 5905       | 31249     | 2           | 248MB        | 44301MB        |
| upstream | All           | hole fit suggestions           | True    | 50      | 1.99    | 0.00  | 39.23  | 0.03     | 2.95      | 0.37        | 39.27  | 19         | 16           | 1369         | 5910       | 31261     | 51          | 325MB        | 162837MB       |
| HEAD     | All           | hole fit suggestions           | True    | 50      | 2.08    | 0.00  | 41.86  | 0.02     | 3.29      | 0.39        | 41.88  | 19         | 16           | 1369         | 5910       | 31261     | 51          | 327MB        | 186180MB       |
|          |               |                                |         |         |         |       |        |          |           |             |        |            |              |              |            |           |             |              |                |

[soulomoon]

The result is generally better with large rule changes cases.
But worse for no change rule visited.

Since we are waiting for the conditional rule to yield a result when refreshing the dependency. No change visit would have a degeneration.

But with rule change, the unneeded rule won't run, thus cancel out the performance degeneration.

Would love to hear you guys opinion on this @pepeiborra @wz1000

[soulomoon]

Otherthing still quit a mess, the importance commit at 8a08863

[soulomoon]

Some good news, I have observed less failure in this branch than the master branch.

[wz1000]

I don't think we need to complicate the API by introducing side conditions - we just need to try evaluating dependencies in order. This means instead of maintaining a KeySet of dependencies, we need to maintain something like a [KeySet]. When building a rule, we pop the first element of the [KeySet], and try building it in parallel - if that succeeds we proceed with the other elements, but if those fail we give up on rebuilding previous dependencies and kick off a fresh build.

[soulomoon]

I don't think we need to complicate the API by introducing side conditions - we just need to try evaluating dependencies in order. This means instead of maintaining a KeySet of dependencies, we need to maintain something like a [KeySet]. When building a rule, we pop the first element of the [KeySet], and try building it in parallel - if that succeeds we proceed with the other elements, but if those fail we give up on rebuilding previous dependencies and kick off a fresh build.

On the above description, it is not clear to me how evaluating dependencies in order can avoid some unwanted Key(branch out) in the latter key set? We need to know which key we are branching on and kick a fresh rebuild on branch condition changed. We still need some explicit annotation?

[wz1000]

rules can be evaluated in 2 modes - if the rule was previously run, then we build all of its dependencies from the previous run and then run (or skip) the rule according to its recomputation flags (is it always rerun etc..).

If the rule was not previously run, then we build it without building its previous dependencies (we don't have any!). The rule might request some dependencies to be built, and we build those on demand by suspending the evaluation of the current rule when this happens.

I'm proposing in case 1 (the rule was previously run), instead of building all dependencies immediately in parallel, we build them in order, and if any of them change, we skip building the subsequent dependencies and just build the rule itself, as in the second case.

[wz1000]

Crucially, if an earlier dependency changes, we skip speculatively building later dependencies. This prevents us from getting into conditions where rules are built when their preconditions are not met because those preconditions should be captured by a change in an earlier dependency.

[soulomoon]

Oh i see how it goes, it would means kicking up deps linearly. It does solve the problem in a more direct way, but I am a bit worried on the performance impact on this linearity.

[wz1000]

For a concrete example, consider the GetLinkable rule, which should only be executed when the NeedsCompilation rule returns a Just value. I think the bug is that in certain cases, initially NeedsCompilation returns a Just value, but then it changes to return Nothing. However, when we evaluate a rule for the second time, we see that it depended on NeedsCompilation and GetLinkable the first time, so we try to build both before proceeding with the rule. This is wrong! If NeedsCompilation changed, we do not want to build GetLinkable! That is the behaviour we need.

[wz1000]

but I am a bit worried on the performance impact on this linearity.

Yes that was the concern, but we could still build all rules depended on simultaneously using the uses family of functions as well as the applicative combinators (<*>) in parallel. It's just that all the parallelism would have to be explicit in the rule definition.

If the performance impact is still too great, we could try doing "true" speculative builds, where we ignore errors and diagnostics from speculative builds of dependencies.

[soulomoon]

Let try a linear approach and do some benchmark to see how much we lost.

But I doubt the speculative builds would be better than annotating the dependencies with an explicit branching dependency tree.

[wz1000]

I guess we could also just throw exceptions when preconditions are violated, and have these exceptions be caught and ignored by hls-graph.

Instead of

error $ "called GetLinkable for a file without a linkable: " ++ show f

throwIO $ PreconditionViolated $ "called GetLinkable for a file without a linkable: " ++ show f

[soulomoon]

I guess we could also just throw exceptions when preconditions are violated, and have these exceptions be caught and ignored by hls-graph.

Instead of

error $ "called GetLinkable for a file without a linkable: " ++ show f

throwIO $ PreconditionViolated $ "called GetLinkable for a file without a linkable: " ++ show f

This is indeed a good add on, and we can benefit from it also collecting the precondition violation statitstic in bench.
But overall all I believe it is long term beneficial for us to improve hls-graph to a point that such violation can be avoided as much as possible when defining a rule but without sabotage the performance too much.

[soulomoon]

Yes that was the concern, but we could still build all rules depended on simultaneously using the uses family of functions as well as the applicative combinators (<*>) in parallel. It's just that all the parallelism would have to be explicit in the rule definition.

It sounds good.

linear deps in https://github.com/soulomoon/haskell-language-server/tree/linearity-deps.
I am running some bench to see how it goes.

[soulomoon]

Also, I have added a test case to prevent phantom dependencies from occuring again in hls-graph.

[jhrcek]

These eval plugin tests https://github.com/haskell/haskell-language-server/actions/runs/8018144432/job/21903462404?pr=4087 usually fail due to the GetLinkable rule failure, so not sure how much this improves the issue.
I'll go over this PR tomorrow and try to see how much it improves the failure rate:

[soulomoon]

In the end I got the (improved) error after 180 iterations of running that test. But it seems to occur less frequently now. "message": "called GetLinkable for a file without a linkable: NormalizedFilePath "/tmp/extra-dir-10246150986263/TI_Info.hs"\nCallStack (from HasCallStack):\n error, called at src/Development/IDE/Core/Rules.hs:1120:18 in ghcide-2.6.0.0-inplace:Development.IDE.Core.Rules"

I guess we can also try to see if this error still happened ? @jhrcek

[jhrcek]

What about this commented out code?
Do we need to keep it? Does something break when you uncomment it?

[soulomoon]

These two are small files, the other two are large files. Just conveniently choosing between them.

[jhrcek]

Can you please explain what does kicking mean in this context?
Also it's not clear to me what you mean by linearly. You're referring to linear order of something, but what's not clear to me is: linear order "of what"?

[soulomoon]

we used to try to kick all the deps concurrently and collect all the results. The linearly means one following another. Following the deps order in the action when it last ran

[soulomoon]

Kicking means firing up the refresh of the dependency, if we have clean cache, it might be just a look up.

[wz1000]

make it clear that these are stored in reverse order

[soulomoon]

Ok, I added a comment about this invariant.
and it also occurred to me that AlwaysRerunDeps should be recovered to KeySet, since it would be one way down to AlwaysRerunDeps on <>, and only ResultDeps need to maintain this invariant.

[wz1000]

I would like to test this out locally before we merge, as there are a lot of tricky properties we need to maintain.

I'll try to do this in the next couple of days,

[soulomoon]

I would like to test this out locally before we merge, as there are a lot of tricky properties we need to maintain.

I'll try to do this in the next couple of days,

thanx, take your time.

[soulomoon]

@wz1000 Is there still some test you want to do locally ? If not, I am considering resolve the conflict and merge this week.