Stabilize ModuleDependency descriptions - #1648
Conversation
f934965 to
13ac918
Compare
|
Thank you so much for your contribution, and first of all sorry for the late feedback! The only thing I'd like to test is how much sorting affects the performance, but other than that, I have no concerns that we can integrate your fix soon. |
c47468c to
77dfee9
Compare
|
Thanks for taking a look, and no worries about the timing! I fixed the DCO issue and normalized both commits to the same GitHub identity/sign-off. I also ran a targeted local benchmark on JDK 25 using real ModuleDependency instances built from ArchUnit classes. It compared the previous unsorted description rendering with the sorted rendering in this PR; the figures below are medians from seven alternating rounds:
So the relative cost is visible for large dependency sets, but the absolute overhead stayed around 2 ms even for the largest real module dependency in this sample, and it is limited to the getDescription/reporting path. I hope this helps with the performance assessment. |
| .sorted() | ||
| .map(HasDescription::getDescription) |
There was a problem hiding this comment.
Thanks for your performance measurements already!
As expected, sorting adds some time. 🤷♂️
In an even more extreme test case of
JavaClasses classes = new ClassFileImporter().importPackages("org.hibernate");
ArchModules<?> modules = ArchModules.defineByPackages("org.hibernate.(*)..").modularize(classes);
long t0 = System.nanoTime();
modules.stream()
.flatMap(m -> m.getModuleDependenciesFromSelf().stream())
.forEach(md -> md.getDescription()); // this is where .sorted() now runs
long duration = System.nanoTime() - t0;on hibernate-core 7.4.5.Final (where one ModuleDependency consists of 11 418 classDependencies),
I measured duration to increase from 30±4 ms to 114±6 ms.
We could, however, easily save some time by not sorting Dependency objects, but just their description Strings:
| .sorted() | |
| .map(HasDescription::getDescription) | |
| .map(HasDescription::getDescription) | |
| .sorted() |
In my case that at least brings us down to 88±6 ms.
What do you think?
There was a problem hiding this comment.
Thanks for the suggestion and the measurements! I’ve updated the PR to sort the rendered descriptions and added a regression test for stable ordering. The checks pass locally.
Sort rendered class dependency descriptions lexicographically so module violation text does not depend on set encounter order. Add a regression test that distinguishes description order from Dependency's line-number-first natural order. Fixes TNG#1630 Signed-off-by: DragonFSKY <38503900+DragonFSKY@users.noreply.github.com>
77dfee9 to
c62a6d8
Compare
hankem
left a comment
There was a problem hiding this comment.
Thanks for the contribution and the test that clearly demonstrates which order is considered!
Fixes #1630.
This canonicalizes the emitted class-dependency descriptions lexicographically before joining them, so the same dependency set produces stable
ModuleDependencyreport text independently of the backing set encounter order. The ordering is defined at the textual representation boundary and does not depend onDependency's line-number-first natural order.The regression test imports real classes, forces opposite dependency encounter orders, and verifies that
Dependencynatural order conflicts with description order. It therefore fails both without sorting and with dependency-object sorting, while passing with description-string sorting.Tested with: