Skip to content

Conversation

JordonPhillips
Copy link
Contributor

@JordonPhillips JordonPhillips commented Sep 10, 2025

This adds a new DependencyGraph to smithy-utils, intended for use in the numerous locations where we perform topological sorts.

Performance-wise, it's a bit faster for dense graphs (integrations with lots of dependencies) and a bit slower for sparse graphs. In the sizes we're realistically looking at for integrations (dozens, tops) the difference is insignificant. We're talking about a microsecond or two.

Here's benchmarking output for the old integration sort:
Screenshot 2025-09-11 at 10 32 58

And here's output for the new:
Screenshot 2025-09-11 at 10 26 08

I also plan to add the same sort of ordering capabilities to smithy build plugins, which is why I've gone to the trouble to make this generic.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@JordonPhillips JordonPhillips requested a review from a team as a code owner September 10, 2025 21:26
@JordonPhillips JordonPhillips marked this pull request as draft September 10, 2025 21:26
{
"type": "feature",
"description": "Add a generic dependency graph to smithy-utils to be used for sorting various dependent objects, such as integrations and plugins.",
"pull_requests": []
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gotta update this

@JordonPhillips JordonPhillips force-pushed the plugin-ordering branch 2 times, most recently from 4bd3c5b to 0a992d7 Compare September 12, 2025 14:00
@JordonPhillips JordonPhillips marked this pull request as ready for review September 17, 2025 14:47
Copy link
Contributor

@kstich kstich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know there are a couple other places that use topological sorting, TopologicalShapeSort and TopologicalIndex at least. TopologicalIndex requires pulling cycles out and storing them, so I don't think it can migrate to this. TopologicalShapeSort looks like it could, though.

remaining.add(node.toString());
}
}
throw new IllegalStateException(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using this detailed exception from TopologicalShapeSort -

public static final class CycleException extends RuntimeException {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can use something like that, but I can't use that since smithy-model depends on smithy-utils, not the other way around.

Copy link
Contributor Author

@JordonPhillips JordonPhillips Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this isn't so simple because exceptions can't be generic. Best I can do is Set<?>, which isn't great. But I can point people to findCycles for better info if needed.

@JordonPhillips JordonPhillips force-pushed the plugin-ordering branch 2 times, most recently from 546aeb1 to f794a27 Compare October 10, 2025 09:52
This adds a generic dependency graph to smithy-utils. This is intended
to be used to sort SmithyIntegrations, SmithyBuildPlugins, and
anything else that needs a topological sort.
@JordonPhillips JordonPhillips force-pushed the plugin-ordering branch 2 times, most recently from ac1ee14 to adb6a85 Compare October 13, 2025 17:32
This updates the SmithyIntegration sorting to use the generic
DependencyGraph. It also adds some benchmarking configurations,
which show a minor speed bump for highly dependent integrations
and a major speed bump for highly independent integrations.
This adds the concept of dependencies to build plugins. Plugins can
now declare that they must be run before or after some plugins. This
uses the same dependency graph that is used by SmithyIntegration, but
unlike integrations there is no concept of a "priority". This is
because we may want to be able to run the plugins themselves in
parallell, and such a priority would complicate that.
This changes the DependencyGraph's topological sort to throw a
dedicated exception class so that it can be better handled.
This updates LoaderShapeMap to use DependencyGraph to sort shapes
instead of TopologicalShapeSort. This results in some error messages
changing:

* There is no longer a special message for missing transitive mixins.
* The message for genrically missing mixins is now applied by
  ApplyMixin, which has a slightly different wording and which will
  trigger for each mising mixin.
* The error message for detected cycles will now include the entire
  cycle that a shape is part of, rather than just the edges that it
  is directly connected to.
This deprecates TopologicalShapeSort in favor of DependencyGraph.
The internal implementation is left alone because it does not
inherently enqueue dependencies like DependencyGraph does. This
results in TopologicalShapeSort throwing a cycle exception when
dependencies are missing, which is a behavior that some may
rely on.
@JordonPhillips
Copy link
Contributor Author

Both TopologicalShapeSort and TopologicalIndex could be converted to use DependencyGraph under the hood, but both have quirks that would be cumbersome to replicate.

TopologicalShapeSort doesn't auto-enqueue dependencies, which causes it to report them as being part of a cycle. This is behavior that we did depend on, and it's a public class that I personally know others are using. We don't need that behavior though, so I updated our code that uses it and made it deprecated.

TopologicalIndex reports elementary cycles as PathFinder.Paths. Right now DependencyGraph can find Strongly-Connected-Components, but not elementary cycles. I could add elementary cycle detection as an option, but I still then have to construct those PathFinder.Paths, which would be expensive. On top of everything, it has two sub-sorts (alphabetical and by number of participating cycles) that would be difficult to replicate. And I really don't want to change that order at all as it'll cause a ton of churn. I can give it a shot, but the scope is big enough that I'd prefer to do it in a separate PR

@JordonPhillips JordonPhillips requested a review from kstich October 13, 2025 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants