This is an automated email from the ASF dual-hosted git repository. thiagohp pushed a commit to branch TAP5-2779 in repository https://gitbox.apache.org/repos/asf/tapestry-5.git
commit 7493ad7fc5b6f88238b59b9a8c1abb4b87ff74d4 Author: Thiago H. de Paula Figueiredo <[email protected]> AuthorDate: Sun Jul 7 17:42:29 2024 -0300 Another pass at getting closer to the end of TAP5-2742 --- .../internal/plastic/PlasticClassLoader.java | 41 +---- .../services/ComponentInstantiatorSourceImpl.java | 7 +- .../services/ComponentModelSourceImpl.java | 6 - .../internal/services/PageSourceImpl.java | 11 -- .../apache/tapestry5/modules/PageLoadModule.java | 19 ++- .../services/pageload/PageClassLoaderContext.java | 52 +++++-- .../PageClassLoaderContextManagerImpl.java | 171 +++++++++++++-------- 7 files changed, 174 insertions(+), 133 deletions(-) diff --git a/plastic/src/main/java/org/apache/tapestry5/internal/plastic/PlasticClassLoader.java b/plastic/src/main/java/org/apache/tapestry5/internal/plastic/PlasticClassLoader.java index c0774c13a..591ba3729 100644 --- a/plastic/src/main/java/org/apache/tapestry5/internal/plastic/PlasticClassLoader.java +++ b/plastic/src/main/java/org/apache/tapestry5/internal/plastic/PlasticClassLoader.java @@ -41,10 +41,6 @@ public class PlasticClassLoader extends ClassLoader private Function<String, Class<?>> alternativeClassloading; - private Consumer<String> beforeLoadClass; - - private Set<String> loadedClasses; - private String tag; private Map<String, Class<?>> cache; @@ -55,7 +51,6 @@ public class PlasticClassLoader extends ClassLoader { super(parent); this.delegate = delegate; - loadedClasses = new HashSet<>(); } @Override @@ -71,10 +66,6 @@ public class PlasticClassLoader extends ClassLoader if (shouldInterceptClassLoading(name)) { - if (name.equals("org.apache.tapestry5.integration.app1.pages.GridDemo")) { - System.out.println(); - } - Class<?> c = getFromCache(name); if (c == null) @@ -82,19 +73,7 @@ public class PlasticClassLoader extends ClassLoader if ((filter != null && filter.test(name)) || (filter == null && delegate.shouldInterceptClassLoading(name))) { - - try { - log.add(String.format("Loading %s in %s", name, this)); - c = delegate.loadAndTransformClass(name); - } - catch (LinkageError e) { - e.printStackTrace(); - for (String entry : log) { - System.out.println(entry); - } - System.out.println(); - throw e; - } + c = delegate.loadAndTransformClass(name); } else if (alternativeClassloading != null) { @@ -113,9 +92,6 @@ public class PlasticClassLoader extends ClassLoader return super.loadClass(name, resolve); } - -// loadedClasses.add(name + " : " + System.currentTimeMillis()); - if (resolve) resolveClass(c); @@ -136,10 +112,6 @@ public class PlasticClassLoader extends ClassLoader { synchronized(getClassLoadingLock(className)) { - if (className.equals("org.apache.tapestry5.integration.app1.pages.GridDemo")) - { - System.out.println(); - } return defineClass(className, bytecode, 0, bytecode.length); } } @@ -205,15 +177,4 @@ public class PlasticClassLoader extends ClassLoader return c; } -// /** -// * Sets a {@linkplain Consumer} that will be called before a transformed -// * class is loaded. -// * @param beforeLoadClass the <code>Consumer<String></code>. -// * @since 5.8.7 -// */ -// public void setBeforeLoadClass(Consumer<String> beforeLoadClass) -// { -// this.beforeLoadClass = beforeLoadClass; -// } - } diff --git a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ComponentInstantiatorSourceImpl.java b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ComponentInstantiatorSourceImpl.java index 4a2b296b5..5b7f1ff99 100644 --- a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ComponentInstantiatorSourceImpl.java +++ b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ComponentInstantiatorSourceImpl.java @@ -399,8 +399,8 @@ public final class ComponentInstantiatorSourceImpl implements ComponentInstantia final Set<String> dependencies = new HashSet<>(); dependencies.addAll( componentDependencyRegistry.getDependencies(className, DependencyType.USAGE)); -// dependencies.addAll( -// componentDependencyRegistry.getDependencies(className, DependencyType.SUPERCLASS)); + dependencies.addAll( + componentDependencyRegistry.getDependencies(className, DependencyType.SUPERCLASS)); for (String dependency : dependencies) { if (!OPEN_INSTANTIATORS.get().contains(dependency)) @@ -429,9 +429,6 @@ public final class ComponentInstantiatorSourceImpl implements ComponentInstantia ClassInstantiator<Component> plasticInstantiator; try { - if (className.equals("org.apache.tapestry5.integration.app1.pages.GridInLoopDemo")) { - System.out.println(); - } plasticInstantiator = context.getPlasticManager().getClassInstantiator(className); if (multipleClassLoaders) { diff --git a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ComponentModelSourceImpl.java b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ComponentModelSourceImpl.java index b167b39ba..918d05e87 100644 --- a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ComponentModelSourceImpl.java +++ b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ComponentModelSourceImpl.java @@ -52,12 +52,6 @@ public class ComponentModelSourceImpl implements ComponentModelSource if (multipleClassLoaders && isPage(componentClassName)) { - if (componentClassName.contains("GridDemo") || - componentClassName.contains("GridInLoopDemo")) - { - System.out.println(); - } - final Set<String> superclasses = componentDependencyRegistry.getDependencies( componentClassName, DependencyType.SUPERCLASS); diff --git a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/PageSourceImpl.java b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/PageSourceImpl.java index 6c67fe802..f2f6a34b0 100644 --- a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/PageSourceImpl.java +++ b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/PageSourceImpl.java @@ -273,17 +273,6 @@ public class PageSourceImpl implements PageSource } } - // TODO: remove -// for (String pageClassName : pageDependencies) -// { -// try -// { -// pageClassLoaderContextManager.get(pageClassName).getClassLoader().loadClass(pageClassName); -// } catch (ClassNotFoundException e) -// { -// throw new RuntimeException(e); -// } -// } } @PostInjection diff --git a/tapestry-core/src/main/java/org/apache/tapestry5/modules/PageLoadModule.java b/tapestry-core/src/main/java/org/apache/tapestry5/modules/PageLoadModule.java index b658553bb..4d09743d5 100644 --- a/tapestry-core/src/main/java/org/apache/tapestry5/modules/PageLoadModule.java +++ b/tapestry-core/src/main/java/org/apache/tapestry5/modules/PageLoadModule.java @@ -47,6 +47,7 @@ import org.apache.tapestry5.services.pageload.PagePreloader; import org.apache.tapestry5.services.pageload.PreloaderMode; import org.apache.tapestry5.services.pageload.ReferenceType; import org.apache.tapestry5.services.templates.ComponentTemplateLocator; +import org.slf4j.LoggerFactory; /** * @since 5.3 @@ -97,7 +98,23 @@ public class PageLoadModule { if (!productionMode && multipleClassLoaders) { - pageClassLoaderContextManager.preloadContexts(); + // If we have component dependency information previously stored in + // a file, then we just preload the page classloader contexts. + // Otherwise, we gather component dependency information then + // preload the page classloader contexts. + if (componentDependencyRegistry.isStoredDependencyInformationPresent()) + { + pageClassLoaderContextManager.preloadContexts(); + } + else + { + LoggerFactory.getLogger(PageClassLoaderContextManager.class) + .warn("If the component dependency process is taking too long, " + + "consider writing its results to a file using the " + + " 'Store dependency information' button " + + "in the /t5dashboard/pages page."); + pageClassLoaderContextManager.preload(); + } } // Preload the dependency information for all pages // when in production mode. Without that, exceptions during diff --git a/tapestry-core/src/main/java/org/apache/tapestry5/services/pageload/PageClassLoaderContext.java b/tapestry-core/src/main/java/org/apache/tapestry5/services/pageload/PageClassLoaderContext.java index 8ae71f5f4..8d914be9c 100644 --- a/tapestry-core/src/main/java/org/apache/tapestry5/services/pageload/PageClassLoaderContext.java +++ b/tapestry-core/src/main/java/org/apache/tapestry5/services/pageload/PageClassLoaderContext.java @@ -22,7 +22,6 @@ import java.util.function.Function; import org.apache.tapestry5.commons.services.PlasticProxyFactory; import org.apache.tapestry5.internal.plastic.PlasticClassLoader; -import org.apache.tapestry5.internal.plastic.PlasticClassPool; import org.apache.tapestry5.plastic.PlasticManager; import org.apache.tapestry5.plastic.PlasticUtils; import org.slf4j.Logger; @@ -316,35 +315,70 @@ public class PageClassLoaderContext PageClassLoaderContext other = (PageClassLoaderContext) obj; return Objects.equals(name, other.name); } - + @Override public String toString() { + return toString(true); + } + + public String toString(boolean includeClassNames) + { + final PlasticClassLoader classLoader = (PlasticClassLoader) proxyFactory.getClassLoader(); return "PageClassloaderContext [name=" + name + ", parent=" + (parent != null ? parent.getName() : "null" ) + - ", classLoader=" + afterAt(proxyFactory.getClassLoader().toString()) + - (isRoot() ? "" : ", classNames=" + classNames) + + ", classLoader=" + afterAt(classLoader.getClassLoaderId()) + + (isRoot() || !includeClassNames ? "" : ", classNames=" + classNames) + "]"; } public String toRecursiveString() + { + return toRecursiveString(true); + } + + public String toRecursiveString(boolean outputClasses) { StringBuilder builder = new StringBuilder(); - toRecursiveString(builder, ""); + toRecursiveString(builder, "", outputClasses); return builder.toString(); } - private void toRecursiveString(StringBuilder builder, String tabs) + public final boolean isEqualOrAncestor(PageClassLoaderContext dependencyContext) + { + boolean equalOrAncestor = this.equals(dependencyContext); + if (!equalOrAncestor) + { + PageClassLoaderContext parent = this.getParent(); + while (parent != null && !equalOrAncestor) + { + equalOrAncestor = parent.equals(dependencyContext); + if (equalOrAncestor) + { + break; + } + else + { + parent = parent.getParent(); + } + } + } + return equalOrAncestor; + } + + private void toRecursiveString(StringBuilder builder, String tabs, boolean outputClasses) { builder.append(tabs); builder.append(name); builder.append(" : "); builder.append(afterAt(proxyFactory.getClassLoader().toString())); - builder.append(" : "); - builder.append(classNames); + if (outputClasses) { + builder.append(" : "); + builder.append(classNames); + } builder.append("\n"); for (PageClassLoaderContext child : children) { - child.toRecursiveString(builder, tabs + "\t"); + child.toRecursiveString(builder, tabs + "\t", outputClasses); } } diff --git a/tapestry-core/src/main/java/org/apache/tapestry5/services/pageload/PageClassLoaderContextManagerImpl.java b/tapestry-core/src/main/java/org/apache/tapestry5/services/pageload/PageClassLoaderContextManagerImpl.java index 7a3df12f7..123e356b2 100644 --- a/tapestry-core/src/main/java/org/apache/tapestry5/services/pageload/PageClassLoaderContextManagerImpl.java +++ b/tapestry-core/src/main/java/org/apache/tapestry5/services/pageload/PageClassLoaderContextManagerImpl.java @@ -15,6 +15,7 @@ package org.apache.tapestry5.services.pageload; import java.util.ArrayList; import java.util.Collections; +import java.util.Comparator; import java.util.HashSet; import java.util.List; import java.util.Objects; @@ -28,10 +29,7 @@ import org.apache.tapestry5.SymbolConstants; import org.apache.tapestry5.commons.internal.util.TapestryException; import org.apache.tapestry5.commons.services.InvalidationEventHub; import org.apache.tapestry5.commons.services.PlasticProxyFactory; -import org.apache.tapestry5.internal.TapestryInternalUtils; import org.apache.tapestry5.internal.ThrowawayClassLoader; -import org.apache.tapestry5.internal.plastic.ClassLoaderDelegate; -import org.apache.tapestry5.internal.plastic.PlasticClassLoader; import org.apache.tapestry5.internal.services.ComponentDependencyRegistry; import org.apache.tapestry5.internal.services.ComponentDependencyRegistry.DependencyType; import org.apache.tapestry5.internal.services.InternalComponentInvalidationEventHub; @@ -70,12 +68,12 @@ public class PageClassLoaderContextManagerImpl implements PageClassLoaderContext private static final AtomicInteger MERGED_COUNTER = new AtomicInteger(1); - private final static ThreadLocal<Boolean> CONTEXTS_CHANGED = ThreadLocal.withInitial(() -> false); + private final static ThreadLocal<AtomicInteger> CONTEXTS_CREATED = ThreadLocal.withInitial(AtomicInteger::new); private Function<ClassLoader, PlasticProxyFactory> plasticProxyFactoryProvider; private PageClassLoaderContext root; - + public PageClassLoaderContextManagerImpl( final ComponentDependencyRegistry componentDependencyRegistry, final ComponentClassResolver componentClassResolver, @@ -252,20 +250,45 @@ public class PageClassLoaderContextManagerImpl implements PageClassLoaderContext alreadyProcessed.add(className); - // Sorting dependencies alphabetically so we have consistent results. + // Sorting dependencies by type/alphabetically so we have consistent + // context trees between runs of the same webapp + List<String> allNonPageDependencies = new ArrayList<>( + componentDependencyRegistry.getAllNonPageDependencies(className)); + Collections.sort(allNonPageDependencies, ClassNameComparator.INSTANCE); + List<String> dependencies = new ArrayList<>(getDependenciesWithoutPages(className)); - Collections.sort(dependencies); + Collections.sort(dependencies, ClassNameComparator.INSTANCE); // Process dependencies depth-first - for (String dependency : dependencies) + do { - // Avoid infinite recursion loops - if (!alreadyProcessed.contains(dependency)) + + // Very unlikely to have infinite loops, but lets + // avoid them anyway. + int passes = 0; + + int contextsCreatedInThisPass = -1; + + while (contextsCreatedInThisPass < CONTEXTS_CREATED.get().get() && passes < 1000) { - processUsingDependencies(dependency, root, unknownContextProvider, - plasticProxyFactoryProvider, classesToInvalidate, alreadyProcessed, false); + + contextsCreatedInThisPass = CONTEXTS_CREATED.get().get(); + + for (String dependency : allNonPageDependencies) + { + // Avoid infinite recursion loops + if (!alreadyProcessed.contains(dependency) + || root.findByClassName(dependency) == null) + { + processUsingDependencies(dependency, root, unknownContextProvider, + plasticProxyFactoryProvider, classesToInvalidate, alreadyProcessed, false); + } + } + } + } + while (!allNeededContextsAvailable(allNonPageDependencies)); // Collect context dependencies Set<PageClassLoaderContext> contextDependencies = new HashSet<>(); @@ -300,6 +323,7 @@ public class PageClassLoaderContextManagerImpl implements PageClassLoaderContext Collections.singleton(className), plasticProxyFactoryProvider.apply(root.getClassLoader()), this::get); + CONTEXTS_CREATED.get().incrementAndGet(); } else { @@ -318,6 +342,7 @@ public class PageClassLoaderContextManagerImpl implements PageClassLoaderContext Collections.singleton(className), plasticProxyFactoryProvider.apply(parentContext.getClassLoader()), this::get); + CONTEXTS_CREATED.get().incrementAndGet(); } if (multipleClassLoaders) @@ -325,39 +350,13 @@ public class PageClassLoaderContextManagerImpl implements PageClassLoaderContext context.getParent().addChild(context); } -// // Ensure non-page class is initialized in the correct context and classloader. -// // Pages get their own context and classloader, so this initialization -// // is both non-needed and a cause for an NPE if it happens. -// if (!componentClassResolver.isPage(className) -// || componentDependencyRegistry.getDependencies(className, DependencyType.USAGE).isEmpty()) -// { -// // Avoiding "attempted duplicate class definition" due to -// // loading a class into a classloader which already loaded -// // that class before. -// if (!context.getClassNames().contains(className)) -// { -// list.get().add(className); -// try { -// loadClass(className, context); -// } -// catch (LinkageError e) { -// System.out.println("-------------------------"); -// for (String c : list.get()) { -// System.out.println(c); -// } -// System.out.println("-------------------------"); -// throw e; -// } -// } -// } - if (multipleClassLoaders) { LOGGER.debug("New context: {}", context); } } - CONTEXTS_CHANGED.set(true); + } } @@ -366,6 +365,20 @@ public class PageClassLoaderContextManagerImpl implements PageClassLoaderContext return context; } + private boolean allNeededContextsAvailable(List<String> dependencies) + { + boolean available = true; + for (String dependency : dependencies) + { + if (root.findByClassName(dependency) == null) + { + available = false; + break; + } + } + return available; + } + private Set<String> getDependenciesWithoutPages(String className) { Set<String> dependencies = new HashSet<>(); @@ -490,6 +503,8 @@ public class PageClassLoaderContextManagerImpl implements PageClassLoaderContext plasticProxyFactoryProvider.apply(parent.getClassLoader()), this::get); + CONTEXTS_CREATED.get().incrementAndGet(); + parent.addChild(merged); // Recreating contexts for classes that got invalidated but @@ -721,13 +736,14 @@ public class PageClassLoaderContextManagerImpl implements PageClassLoaderContext if (LOGGER.isInfoEnabled()) { - LOGGER.info(String.format("Dependency information gathered in %.3f ms", (finish - start) / 1000.0)); + LOGGER.info(String.format("Dependency information for %d pages gathered in %.3f s", + pageNames.size(), (finish - start) / 1000.0)); } preloadContexts(); } - + @Override public void preloadContexts() { @@ -737,44 +753,77 @@ public class PageClassLoaderContextManagerImpl implements PageClassLoaderContext start = System.currentTimeMillis(); - final List<String> classNames = new ArrayList<>(); - classNames.addAll(componentClassResolver.getMixinNames().stream() - .map(s -> componentClassResolver.resolveMixinTypeToClassName(s)) - .sorted() - .collect(Collectors.toList())); - - classNames.addAll(componentClassResolver.getComponentNames().stream() - .map(s -> componentClassResolver.resolveComponentTypeToClassName(s)) - .sorted() - .collect(Collectors.toList())); - - classNames.addAll(componentClassResolver.getPageNames().stream() - .map(s -> componentClassResolver.resolvePageNameToClassName(s)) - .sorted() - .collect(Collectors.toList())); + final List<String> classNames = new ArrayList<>(componentDependencyRegistry.getClassNames()); + classNames.sort(ClassNameComparator.INSTANCE); int runs = 0; // The run counter check is to just avoid possible infinite loops, // although that's very unlikely. - CONTEXTS_CHANGED.set(true); - while (runs < 5000 && CONTEXTS_CHANGED.get() == true) + int contexts = -1; + while (runs < 5000 && contexts < CONTEXTS_CREATED.get().get()) { runs++; - CONTEXTS_CHANGED.set(false); + contexts = CONTEXTS_CREATED.get().get(); for (String className : classNames) { get(className); } - System.out.println(CONTEXTS_CHANGED.get()); +// Collections.reverse(classNames); } finish = System.currentTimeMillis(); if (LOGGER.isInfoEnabled()) { - LOGGER.info(String.format("Preloading of page classloader contexts finished in %.3f ms (%d passes)", (finish - start) / 1000.0, runs)); + LOGGER.info(String.format("Preloading of page classloader contexts finished in %.3f s (%d passes)", (finish - start) / 1000.0, runs)); } } + /** + * Sorts base classes before mixins, mixins before components and components + * before pages. If both classes belong to the same type, order alphabetically. + */ + private static final class ClassNameComparator implements Comparator<String> + { + + private static final Comparator<String> INSTANCE = new ClassNameComparator(); + + @Override + public int compare(String o1, String o2) + { + int value1 = getValue(o1); + int value2 = getValue(o2); + int comparison = value1 - value2; + if (comparison == 0) + { + comparison = o1.compareTo(o2); + } + return comparison; + } + + private int getValue(String className) + { + int value; + if (className.contains(".base.")) + { + value = 0; + } + else if (className.contains(".mixins.")) + { + value = 1; + } + else if (className.contains(".components.")) + { + value = 2; + } + else + { + value = 3; + } + return value; + } + + } + }
