Reimplement shake (continued) #2060
Conversation
|
This seems working now. Performance is a mixed bag, I will look at mining the data later today or tomorrow. Before merging I need to implement reverse dependency tracking and measure scalability. |
PerformanceWithout reverse dependency trackingTotal Time, allocations and max residency are both in the ballpark of Shake, with ups and downs. Using the Cabal example where HEAD is 3dfc695, we have:
With reverse dependency trackingUsing the Cabal example where HEAD is ac99d9c, we see that total time is lower for most examples but max residency is considerably higher, presumably due to the additional state to track reverse dependencies (or a bug):
I will try to rerun the benchmarks in a more isolated environment again. The next step will be to measure scalability |
|
Repeated the benchmarks in a more isolated environment (Xeon gold 6138 w/ 256GB RAM, 80 virtual cores) and noticed some failed runs due to "Index out of range" crashes in this branch. I guess there must be a parallelism bug somewhere that doesn't arise in the 1-core Github CI agents. Will need to investigate more: |
pepeiborra
force-pushed
the
reimplement-shake
branch
from
ae223c1 to
6d917f0
Compare
|
It reproduces reliably with Tracking it down is extremely difficult because |
|
Tracked it down to the GHC thread manager by editing base to use
https://gitlab.haskell.org/ghc/ghc/-/blob/master/libraries/base/GHC/Event/Thread.hs#L182 It looks like |
|
The problem was that we were calling |
pepeiborra
force-pushed
the
reimplement-shake
branch
from
b0b57c2 to
da6230f
Compare
|
I believe the performance regressions are due to the parallel evaluation of all the dependencies of a node. Shake is
1 gets there first and spawns thousands of threads. The chunky build does little actual work: only a few nodes in the build graph need recomputing, all the other ones are just doing a lookup. When 2 arrives it spawns another thread which needs to join the queue at the back, and ends up waiting for a long time. With reverse dependency tracking, the chunky build in 1 becomes much less chunky and the regression should disappear, except in the worst case (startup) when all the project files are FOIs. We could solve this by: |
This is useful in the context of #2060 to compare performance with and without reactive change tracking
This is useful in the context of haskell#2060 to compare performance with and without reactive change tracking
pepeiborra
force-pushed
the
reimplement-shake
branch
2 times, most recently
from
f8985d4 to
6d9b6cd
Compare
* [ghcide-bench] Support extra args in examples This is useful in the context of #2060 to compare performance with and without reactive change tracking * Fix bench.yml CI script
pepeiborra
force-pushed
the
reimplement-shake
branch
7 times, most recently
from
665cafb to
42410bf
Compare
|
Performance regressions have been mitigated via both a) reverse deps and b) avoiding new threads for trivial lookups. Will follow up with benchmark results soon. |
pepeiborra
force-pushed
the
reimplement-shake
branch
2 times, most recently
from
b095f40 to
d652f11
Compare
The main benefit of reverse dependency tracking is that we avoid unnecessary node lookups. However, these lookups were not shown in hls-graph profiles, hiding their real cost. Concretely, if the number of lookups per build is proportional to the number of transitive dependencies of a node, which in turn is proportional to the number of transitive imports of a module, then we have an O(edges) complexity instead of an O(nodes) complexity, which is really bad for large projects. This Diff extends the recorded data and the profiling UI to keep track of visited nodes and to show them in a new column "Visited" of the "Rules" tab. The cost of doing this is storing an additional Int per node at runtime. While I was editing the profiling UI, I took the chance to remove the command tabs, update the README and add some missing files
pepeiborra
force-pushed
the
reimplement-shake
branch
from
4d52c5c to
4d0cc73
Compare
|
Finally! |
Rebase of #1759 including bugfixes
Summary of changes
NeedsCompilationrule)--conservative-change-trackingis added to ghcide to disable reactive change tracking. The flag is intended for debugging only and not for users. A new benchmark example "cabal-conservative" uses it to measure the performance difference.Binaryinstances no longer needed and removed from the codebaseWhat's missing