gnodet-bot commented on code in PR #13319:
URL: https://github.com/apache/maven/pull/13319#discussion_r4276738477
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultTypeRegistry.java:
##########
@@ -78,26 +78,29 @@ public Optional<Type> lookup(String id) {
@Nonnull
public Type require(String id) {
requireNonNull(id, "id cannot be null");
- return usedTypes.computeIfAbsent(id, i -> {
- Type type = lookup.lookupList(TypeProvider.class).stream()
- .flatMap(p -> p.provides().stream())
- .filter(t -> Objects.equals(id, t.id()))
- .findFirst()
- .orElse(null);
- if (type == null) {
- // Copy data as the ArtifactHandler is not immutable, but Type
should be.
- ArtifactHandler handler = manager.getArtifactHandler(id);
- type = new DefaultType(
- id,
-
languageRegistry.lookup(handler.getLanguage()).orElseGet(() ->
handler::getLanguage),
- handler.getExtension(),
- handler.getClassifier(),
- handler.isIncludesDependencies(),
- handler.isAddedToClasspath()
- ? new JavaPathType[] {JavaPathType.CLASSES,
JavaPathType.MODULES}
- : new JavaPathType[] {});
- }
+ // Dynamic providers are queried first (uncached) so that SISU
visibility
+ // filtering works correctly for project-extension-contributed types.
+ // Only the legacy ArtifactHandler fallback is cached.
+ Type type = lookup.lookupList(TypeProvider.class).stream()
Review Comment:
⚠️ **Performance concern:** `lookup.lookupList(TypeProvider.class)` is now
called on every `require()` invocation — outside `computeIfAbsent`, so the SISU
registry is queried unconditionally, bypassing the `usedTypes` cache.
In a large reactor with `-T`, artifact type resolution (`jar`, `pom`, etc.)
is called thousands of times per project. SISU registry lookups involve
internal lock acquisition. This is intentional (the comment says so), but worth
documenting: is SISU's own internal caching sufficient to bound latency under
parallel builds, or should a short-lived per-request memoization be considered
as a safety net?
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/ExtensibleEnumRegistries.java:
##########
@@ -89,15 +90,36 @@ public abstract static class DefaultExtensibleEnumRegistry<
implements ExtensibleEnumRegistry<T> {
protected final Map<String, T> values;
+ private final Lookup lookup;
+ private final Class<P> providerType;
+ @SafeVarargs
public DefaultExtensibleEnumRegistry(List<P> providers, T...
builtinValues) {
+ this(null, null, providers, builtinValues);
+ }
+
+ @SafeVarargs
+ public DefaultExtensibleEnumRegistry(
+ Lookup lookup, Class<P> providerType, List<P> providers, T...
builtinValues) {
+ this.lookup = lookup;
+ this.providerType = providerType;
values = Stream.concat(Stream.of(builtinValues),
providers.stream().flatMap(p -> p.provides().stream()))
.collect(Collectors.toUnmodifiableMap(t ->
t.id().toLowerCase(Locale.ROOT), t -> t));
}
@Override
public Optional<T> lookup(String id) {
- return Optional.ofNullable(values.get(requireNonNull(id,
"id").toLowerCase(Locale.ROOT)));
+ String key = requireNonNull(id, "id").toLowerCase(Locale.ROOT);
+ if (lookup != null && providerType != null) {
+ Optional<T> result = lookup.lookupList(providerType).stream()
Review Comment:
💡 **Minor:** `DefaultExtensibleEnumRegistry.lookup()` now iterates all
registered providers and all their values on every call (O(n·m) scan). For
current cardinalities this is fine, but since this is now a generic base class
shared by three registries, it's worth documenting the cardinality assumption
(e.g. providers and their types are expected to be O(tens), not O(thousands))
so future implementors know the performance boundaries.
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultLifecycleRegistry.java:
##########
@@ -135,6 +144,18 @@ public Iterator<Lifecycle> iterator() {
@Override
public Stream<Lifecycle> stream() {
+ if (lookup != null) {
Review Comment:
ℹ️ **Missing test coverage:** `DefaultLifecycleRegistry` received the same
structural changes as `DefaultTypeRegistry` and `DefaultPackagingRegistry`
(dynamic `Lookup` field, revised `stream()` logic), but no corresponding
`DefaultLifecycleRegistryTest` was added. Both sibling registries have
dedicated new test files. A test covering dynamic-provider visibility
(analogous to `doesNotRetainDynamicTypeAcrossProjects`) would close the gap.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
