Rewrite DependencyGraph [ci: last-only]

[som-snytt]

Improve PhaseAssembly for correctness and performance.

One test correctly assembles the phases now but failed before.

One nuance is that phases at the same "level" (distance from parser) used to be sorted only by name, but now are sorted first by whether they are "internal" or supplied by a plugin. The async test relied on the old sorting but arguably using runsRightAfter is more correct.

Scaladoc was lax about its phasesSet, which is typically the sum of internal, platform, and plugin components. Now Scaladoc only adds the phases it wants.

Some anomalies such as empty phase names (for runsAfter constraints) have been corrected, but will be ignored.

The initial (parser) and terminal (terminal) phases are required. The standard terminal component is added if there is none. All other phases run after parser and before terminal.

The phasesSet is closed; a phase can't depend on a spurious phase name.

Fixes scala/bug#8755

[SethTisue]

what prompted you to pry open the lid of this haunted ancient crypt?

[som-snytt]

Will replace the algo, but thought I'd tease it apart first, also to run the benchmark for before/after. Also, forgot to review dotty phase fusion.

[som-snytt]

"parser" and "terminal" are still special names, but should rely only on initial and terminal.

[lrytz]

This looks a lot clearer - I need more time to look into compilerPhaseList though. Do you write graph algorithm textbooks in your spare time?

Some high-level questions.

I assume it can give different results than 2.13.13? It's not unlikely to have loose dependency graphs, I remember lightbend/genjavadoc#191 from last time. Would it be possible to

• also run the old algorithm, warn if there are changes?
• warn about loose graphs? I guess that's not actionalbe though, when using multiple plugins.

Is the algorithm deterministic? I see the starting point is global.phasesSet, which is a hash set. Would it make sense to sort them in DependencyGraph.apply?

[lrytz]

when do we get here?

[som-snytt]

There is already v -> w (runsBefore) and we add v => w (runsRightAfter), retaining the => (for FollowsNow).

Otherwise, the new edge is normal Follows so keep existing edge, which can't have lower weight.

[lrytz]

and performance

Can you elaborate how the new implementation is better? Did you compare the benchmark before / after? It seems performance is relevant e83ea44

[som-snytt]

I assume CB will show broken constraints (since the compiler had to be tweaked); see the OP comment on async and scaladoc.

For example, I had to accommodate having a runsAfter that duplicates runsRightAfter (which is redundant).

Users could compare old and new by trying the old and new compilers.

This is a topo sort for longest path, where runsRightAfter is special. See OP for sorting behavior at same distance; FSR I tweaked it to prefer "internal" phases first, then plugins sorted by name.

Recently I used github search for language:scala PluginComponent and was not rewarded. I see lihaoyi ecosystem has some plugins, so I'll try those and also your example (genjavadoc as used by akka).

I ran the retronym benchmark FWIW, which was in the context of adding many phases in order. The unit test agrees that the old code is slow with more phases. This either doesn't matter IRL, or matters to anyone running many compilations in the cloud.

retronym benchmark on 2.13.12

[info] Benchmark                        (size)  Mode  Cnt      Score     Error  Units
[info] PhaseAssemblyBenchmark.assemble       1  avgt   10    816.109 ± 111.995  ns/op
[info] PhaseAssemblyBenchmark.assemble       4  avgt   10   2791.529 ± 178.296  ns/op
[info] PhaseAssemblyBenchmark.assemble       8  avgt   10   6148.445 ± 610.413  ns/op
[info] PhaseAssemblyBenchmark.assemble      16  avgt   10  12495.480 ± 247.890  ns/op
[success] Total time: 856 s (14:16), completed Jan 27, 2024, 5:02:34 PM

[info] PhaseAssemblyBenchmark.assemble       1  avgt   10    824.016 ±   26.198  ns/op
[info] PhaseAssemblyBenchmark.assemble       4  avgt   10   2689.058 ±  455.633  ns/op
[info] PhaseAssemblyBenchmark.assemble       8  avgt   10   5787.073 ±   31.035  ns/op
[info] PhaseAssemblyBenchmark.assemble      16  avgt   10  11815.265 ± 1384.701  ns/op
[info] PhaseAssemblyBenchmark.assemble      64  avgt   10  90687.464 ± 3440.683  ns/op
[success] Total time: 1037 s (17:17), completed Feb 10, 2024, 10:57:51 PM

after, also showing a cost for a tweak for correctness

[info] Benchmark                        (size)  Mode  Cnt      Score     Error  Units
[info] PhaseAssemblyBenchmark.assemble       1  avgt   10   1273.694 ± 165.329  ns/op
[info] PhaseAssemblyBenchmark.assemble       4  avgt   10   3360.838 ±  72.838  ns/op
[info] PhaseAssemblyBenchmark.assemble       8  avgt   10   5512.273 ±  95.010  ns/op
[info] PhaseAssemblyBenchmark.assemble      16  avgt   10   9923.611 ± 475.227  ns/op
[info] PhaseAssemblyBenchmark.assemble      64  avgt   10  37914.080 ± 933.747  ns/op
[success] Total time: 1022 s (17:02), completed Feb 11, 2024, 9:01:28 AM

[info] Benchmark                        (size)  Mode  Cnt      Score     Error  Units
[info] PhaseAssemblyBenchmark.assemble       1  avgt   10   1431.521 ±  75.982  ns/op
[info] PhaseAssemblyBenchmark.assemble       4  avgt   10   3947.124 ±  70.060  ns/op
[info] PhaseAssemblyBenchmark.assemble       8  avgt   10   6603.209 ±  89.266  ns/op
[info] PhaseAssemblyBenchmark.assemble      16  avgt   10  11868.508 ± 160.368  ns/op
[info] PhaseAssemblyBenchmark.assemble      64  avgt   10  46191.684 ± 871.991  ns/op
[success] Total time: 1014 s (16:54), completed Feb 13, 2024, 9:43:43 PM

[som-snytt]

re: unordered inputs https://github.com/scala/scala/pull/10687/files#diff-e616cc3a2f3f30e519fdef9dcd1a42322cea0e8c13a5f6fefef2ed59aaed5e38R126

[som-snytt]

sample new order with acyclic.

[info]      phase name  id  description
[info]      ----------  --  -----------
[info]          parser   1  parse source into ASTs, perform simple desugaring
[info]           namer   2  resolve names, attach symbols to named trees
[info]  packageobjects   3  load package objects
[info]           typer   4  the meat and potatoes: type the trees
[info]  superaccessors   5  add super accessors in traits and nested classes
[info]         acyclic   6
[info]      extmethods   7  add extension methods for inline classes

[lrytz]

Assuming the new implementation is correct, the compiler behaves as specified; but there will be valid changes within the constrants that might uncover bugs in compiler plugins, or it can break projects that were the constraints are too lax and everything was working by accident.

Is release notes enough? @SethTisue

re: unordered inputs https://github.com/scala/scala/pull/10687/files#diff-e616cc3a2f3f30e519fdef9dcd1a42322cea0e8c13a5f6fefef2ed59aaed5e38R126

I don't follow; if the test everything is deterministic, but in the compiler the phases go through a hash set.

[som-snytt]

In general, the phases are assigned their greatest distance from parser (longest path); in each equivalence class, the phases are sorted deterministically (by the tuple (isInternal, name)).

In that test, I was setting up the "actual" constraints to fix a bug (in handling followsRightAfter IIRC), and the comment is that shuffling the inputs shouldn't matter. One of the tests shuffles inputs.

BTW, that bug convinced me that simple things are easier to fix. Also, "basically correct except for this bug" is easier to fix than "code with unknown behavior which, like a broken clock, is occasionally correct, but has this erroneous test we don't know how to fix".

Since there is a good chance that any given plugin "works by chance", if we can get an actual snapshot of CB, I would follow up with tweaks. Besides plugins, someone may also use the "internal API" to manage phases, like Scaladoc.

[lrytz]

In general, the phases are assigned their greatest distance from parser (longest path); in each equivalence class, the phases are sorted deterministically (by the tuple (isInternal, name)).

Good, thanks!

[SethTisue]

I don't doubt the new code is cleaner and faster, but I'm not convinced we should merge this, on cost/benefit grounds. Any behavior change here that affects actual plugins — even if the change could technically be classified as a progression rather than regression — is an ecosystem cost. See Lukas's remarks above.

As for potential benefits

• speed: this code isn't actually a noticeable factor in real build times, is it?
• cleanliness: clean code only has tangible benefit if the code is still undergoing or expected to undergo well-motivated change

[som-snytt]

retronym's previous swing was due to a real customer. The unit test is newly commented that the old code took 20 seconds; I forgot to revert the test from 64K because it is fast now.

If you run CB, I'll follow up with mitigations.

[SethTisue]

retronym's previous swing was due to a real customer

Ah! I've contacted him and invited him to comment.

If you run CB, I'll follow up with mitigations

I can definitely do that (but after 2.13.14 ships).

[som-snytt]

Note to self: needs some clean-up and code comments.

For mitigating change, it should revert to the previous sorting (alpha-only, not internal-first).

The API should not require terminal but do some reasonable thing if

1. parser or terminal are missing
2. use the initial and terminal properties instead of phase names
3. handle phases that don't follow anything the same. (currently not tested)

This PR makes everything follow the initial phase; a lack of constraint does not mean "don't run me". I don't remember what the old code intended with "orphan" nodes.

[som-snytt]

I verified that genjavadoc runs correctly, and runs correctly without the tweaks at #8426