From 01b36450208c8bd1d968d80088211b5314779f51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Kubitz?= Date: Wed, 22 Nov 2023 16:23:15 +0100 Subject: [PATCH] Performance: Add public API for Batch Reads in UI - closes #1614 During Batches: * cache Zip Files * enable JavaModelManager.temporaryCache Also: * uses Batch Reads during some core actions that benefit from it. * adds trace logging when caching is missing. https://github.com/eclipse-jdt/eclipse.jdt.core/issues/1614 --- org.eclipse.jdt.core/.options | 3 + .../org/eclipse/jdt/core/dom/ASTParser.java | 4 + .../model/org/eclipse/jdt/core/JavaCore.java | 76 +++++++++++++++++++ .../jdt/internal/core/JavaModelManager.java | 67 +++++++++++++++- .../internal/core/SearchableEnvironment.java | 3 +- 5 files changed, 149 insertions(+), 4 deletions(-) diff --git a/org.eclipse.jdt.core/.options b/org.eclipse.jdt.core/.options index 0262c1e3a1e..42187bb6e5b 100644 --- a/org.eclipse.jdt.core/.options +++ b/org.eclipse.jdt.core/.options @@ -83,6 +83,9 @@ org.eclipse.jdt.core/debug/selection=false # Reports access to zip and jar files through the Java model org.eclipse.jdt.core/debug/zipaccess=false +# Reports repetitive opening of zip and jar files through the Java model without caching +org.eclipse.jdt.core/debug/zipaccessWarning=false + # Reports the time to perform code completion. org.eclipse.jdt.core/perf/completion=300 diff --git a/org.eclipse.jdt.core/dom/org/eclipse/jdt/core/dom/ASTParser.java b/org.eclipse.jdt.core/dom/org/eclipse/jdt/core/dom/ASTParser.java index 6128a610bd0..2a805f0a0f4 100644 --- a/org.eclipse.jdt.core/dom/org/eclipse/jdt/core/dom/ASTParser.java +++ b/org.eclipse.jdt.core/dom/org/eclipse/jdt/core/dom/ASTParser.java @@ -47,6 +47,7 @@ import org.eclipse.jdt.internal.core.BinaryType; import org.eclipse.jdt.internal.core.ClassFileWorkingCopy; import org.eclipse.jdt.internal.core.DefaultWorkingCopyOwner; +import org.eclipse.jdt.internal.core.JavaModelManager; import org.eclipse.jdt.internal.core.PackageFragment; import org.eclipse.jdt.internal.core.dom.util.DOMASTUtil; import org.eclipse.jdt.internal.core.util.CodeSnippetParsingUtil; @@ -1116,6 +1117,9 @@ public IBinding[] createBindings(IJavaElement[] elements, IProgressMonitor monit } private ASTNode internalCreateAST(IProgressMonitor monitor) { + return JavaModelManager.cacheZipFiles(() -> internalCreateASTCached(monitor)); + } + private ASTNode internalCreateASTCached(IProgressMonitor monitor) { boolean needToResolveBindings = (this.bits & CompilationUnitResolver.RESOLVE_BINDING) != 0; switch(this.astKind) { case K_CLASS_BODY_DECLARATIONS : diff --git a/org.eclipse.jdt.core/model/org/eclipse/jdt/core/JavaCore.java b/org.eclipse.jdt.core/model/org/eclipse/jdt/core/JavaCore.java index 7d50caa2419..09115ab0d94 100644 --- a/org.eclipse.jdt.core/model/org/eclipse/jdt/core/JavaCore.java +++ b/org.eclipse.jdt.core/model/org/eclipse/jdt/core/JavaCore.java @@ -5963,6 +5963,9 @@ public static void run(IWorkspaceRunnable action, IProgressMonitor monitor) thro * @since 3.0 */ public static void run(IWorkspaceRunnable action, ISchedulingRule rule, IProgressMonitor monitor) throws CoreException { + if (readOnly.get().booleanValue()) { + throw new IllegalStateException("Its not allow to modify JavaModel during ReadOnly action."); //$NON-NLS-1$ + } IWorkspace workspace = ResourcesPlugin.getWorkspace(); if (workspace.isTreeLocked()) { new BatchOperation(action).run(monitor); @@ -5971,6 +5974,79 @@ public static void run(IWorkspaceRunnable action, ISchedulingRule rule, IProgres workspace.run(new BatchOperation(action), rule, IWorkspace.AVOID_UPDATE, monitor); } } + /** + * @since 3.37 + */ + @FunctionalInterface + public static interface JavaCallable { + /** + * Computes a value or throws an exception. + * + * @return the result + * @throws E the Exception of given type + */ + V call() throws E; + } + /** + * @since 3.37 + */ + @FunctionalInterface + public static interface JavaRunnable { + /** + * Runs or throws an exception. + * + * @throws E the Exception of given type + */ + void run() throws E; + } + + private static ThreadLocal readOnly = ThreadLocal.withInitial(() -> Boolean.FALSE); + + /** + * Calls the argument and returns its result or its Exception. The argument's {@code call()} is supposed to query + * Java model and must not modify it. This method will try to run Java Model queries in optimized way (caching + * things during the operation). It is safe to nest multiple calls - but not necessary. + * + * + * @param callable + * A JavaCallable that can throw an Exception + * @return the result + * @exception E + * An Exception that is thrown by the argument. + * @since 3.37 + */ + public static T callReadOnly(JavaCallable callable) throws E { + if (readOnly.get().booleanValue()) { + // nested + return callable.call(); + } else { + try { + readOnly.set(Boolean.TRUE); + return JavaModelManager.getJavaModelManager().callReadOnly(callable); + } finally { + readOnly.set(Boolean.FALSE); + } + } + } + + /** + * Runs the argument and will forward its Exception. The argument's {@code run()} is supposed to query Java model + * and must not modify it. This method will try to run Java Model queries in optimized way (caching things during + * the operation). It is safe to nest multiple calls - but not necessary. + * + * @param runnable + * A JavaRunnable that can throw an Exception + * @exception E + * An Exception that is thrown by the argument. + * @since 3.37 + */ + public static void runReadOnly(JavaRunnable runnable) throws E { + callReadOnly(() -> { + runnable.run(); + return null; + }); + } + /** * Bind a container reference path to some actual containers (IClasspathContainer). * This API must be invoked whenever changes in container need to be reflected onto the JavaModel. diff --git a/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/JavaModelManager.java b/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/JavaModelManager.java index a8cc66cf63f..a3646162384 100644 --- a/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/JavaModelManager.java +++ b/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/JavaModelManager.java @@ -39,8 +39,11 @@ import java.io.StringReader; import java.net.URI; import java.text.MessageFormat; +import java.time.Instant; +import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Collections; +import java.util.Deque; import java.util.Enumeration; import java.util.HashMap; import java.util.HashSet; @@ -126,6 +129,7 @@ import org.eclipse.jdt.core.JavaCore; import org.eclipse.jdt.core.JavaModelException; import org.eclipse.jdt.core.WorkingCopyOwner; +import org.eclipse.jdt.core.JavaCore.JavaCallable; import org.eclipse.jdt.core.compiler.CharOperation; import org.eclipse.jdt.core.compiler.CompilationParticipant; import org.eclipse.jdt.core.compiler.IProblem; @@ -377,6 +381,7 @@ public void setCache(IPath path, ZipFile zipFile) { private static final String CP_RESOLVE_ADVANCED_DEBUG = JavaCore.PLUGIN_ID + "/debug/cpresolution/advanced" ; //$NON-NLS-1$ private static final String CP_RESOLVE_FAILURE_DEBUG = JavaCore.PLUGIN_ID + "/debug/cpresolution/failure" ; //$NON-NLS-1$ private static final String ZIP_ACCESS_DEBUG = JavaCore.PLUGIN_ID + "/debug/zipaccess" ; //$NON-NLS-1$ + private static final String ZIP_ACCESS_WARNING_DEBUG_ = JavaCore.PLUGIN_ID + "/debug/zipaccessWarning" ; //$NON-NLS-1$ private static final String DELTA_DEBUG =JavaCore.PLUGIN_ID + "/debug/javadelta" ; //$NON-NLS-1$ private static final String DELTA_DEBUG_VERBOSE =JavaCore.PLUGIN_ID + "/debug/javadelta/verbose" ; //$NON-NLS-1$ private static final String DOM_AST_DEBUG = JavaCore.PLUGIN_ID + "/debug/dom/ast" ; //$NON-NLS-1$ @@ -1682,6 +1687,7 @@ public String toString() { public static boolean CP_RESOLVE_VERBOSE = false; public static boolean CP_RESOLVE_VERBOSE_ADVANCED = false; public static boolean CP_RESOLVE_VERBOSE_FAILURE = false; + public static boolean ZIP_ACCESS_WARNING= false; public static boolean ZIP_ACCESS_VERBOSE = false; public static boolean JRT_ACCESS_VERBOSE = false; @@ -1986,6 +1992,7 @@ public void optionsChanged(DebugOptions options) { BasicSearchEngine.VERBOSE = debug && options.getBooleanOption(SEARCH_DEBUG, false); SelectionEngine.DEBUG = debug && options.getBooleanOption(SELECTION_DEBUG, false); JavaModelManager.ZIP_ACCESS_VERBOSE = debug && options.getBooleanOption(ZIP_ACCESS_DEBUG, false); + JavaModelManager.ZIP_ACCESS_WARNING = debug && options.getBooleanOption(ZIP_ACCESS_WARNING_DEBUG_, false); SourceMapper.VERBOSE = debug && options.getBooleanOption(SOURCE_MAPPER_DEBUG_VERBOSE, false); DefaultCodeFormatter.DEBUG = debug && options.getBooleanOption(FORMATTER_DEBUG, false); @@ -2976,19 +2983,50 @@ public ZipFile getZipFile(IPath path) throws CoreException { */ public static boolean throwIoExceptionsInGetZipFile = false; + /** for tracing only **/ + private final ThreadLocal>> lastAccessByPath = ThreadLocal.withInitial(HashMap::new); + /** for tracing only **/ + private final ThreadLocal lastWarning = new ThreadLocal<>(); + public ZipFile getZipFile(IPath path, boolean checkInvalidArchiveCache) throws CoreException { if (checkInvalidArchiveCache) { isArchiveStateKnownToBeValid(path); } ZipCache zipCache; ZipFile zipFile; - if ((zipCache = this.zipFiles.get()) != null - && (zipFile = zipCache.getCache(path)) != null) { + if ((zipCache = this.zipFiles.get()) != null && (zipFile = zipCache.getCache(path)) != null) { return zipFile; } File localFile = getLocalFile(path); - try { + if (ZIP_ACCESS_WARNING) { + Instant now = Instant.now(); + Deque lastAcesses = this.lastAccessByPath.get().compute(path, + (p, l) -> (l == null) ? new ArrayDeque<>() : l); + Instant recentAccess = lastAcesses.peekFirst(); + Instant lastAccess = lastAcesses.peekLast(); + lastAcesses.offerLast(now); + if (lastAccess != null) { + long elapsedMs = lastAccess.until(now, java.time.temporal.ChronoUnit.MILLIS); + if (elapsedMs <= 100000) { + trace(path + " opened again in this thread after " + elapsedMs + "ms"); //$NON-NLS-1$ //$NON-NLS-2$ + } + } + // only warn if there have recently multiple accesses, because it's common but not that bad to have 2 of them + if (recentAccess != null && lastAcesses.size() > 2) { + long elapsedMs = recentAccess.until(now, java.time.temporal.ChronoUnit.MILLIS); + lastAcesses.pollFirst(); + Instant lastInstant = this.lastWarning.get(); + long elapsedWarningMs = lastInstant == null ? Long.MAX_VALUE + : lastInstant.until(now, java.time.temporal.ChronoUnit.MILLIS); + if (elapsedMs < 100 && elapsedWarningMs > 1000) { + this.lastWarning.set(now); + new Exception("Zipfile was opened multiple times wihtin " + elapsedMs + "ms in same thread " //$NON-NLS-1$ //$NON-NLS-2$ + + Thread.currentThread() + ", consider caching: " + path) //$NON-NLS-1$ + .printStackTrace(); + } + } + } if (ZIP_ACCESS_VERBOSE) { trace("(" + Thread.currentThread() + ") [JavaModelManager.getZipFile(IPath)] Creating ZipFile on " + localFile ); //$NON-NLS-1$ //$NON-NLS-2$ } @@ -5727,4 +5765,27 @@ public ClasspathAccessRule getAccessRuleForProblemId(char [] filePattern, int pr private ClasspathAccessRule getFromCache(ClasspathAccessRule rule) { return DeduplicationUtil.internObject(rule); } + + public T callReadOnly(JavaCallable callable) throws E { + boolean hadTemporaryCache = hasTemporaryCache(); + try { + getTemporaryCache(); + + return cacheZipFiles(callable); + } finally { + if (!hadTemporaryCache) { + resetTemporaryCache(); + } + } + } + + public static T cacheZipFiles(JavaCallable callable) throws E { + Object instance = new Object(); + try { + getJavaModelManager().cacheZipFiles(instance); + return callable.call(); + } finally { + getJavaModelManager().flushZipFiles(instance); + } + } } diff --git a/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/SearchableEnvironment.java b/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/SearchableEnvironment.java index 0825706358d..8c64934ab8d 100644 --- a/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/SearchableEnvironment.java +++ b/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/SearchableEnvironment.java @@ -104,7 +104,8 @@ public SearchableEnvironment(JavaProject project, org.eclipse.jdt.core.ICompilat || !JavaCore.IGNORE.equals(project.getOption(JavaCore.COMPILER_PB_DISCOURAGED_REFERENCE, true)); this.workingCopies = workingCopies; this.nameLookup = project.newNameLookup(workingCopies, excludeTestCode); - boolean java9plus = CompilerOptions.versionToJdkLevel(project.getOption(JavaCore.COMPILER_COMPLIANCE, true)) >= ClassFileConstants.JDK9; + boolean java9plus = JavaCore.callReadOnly(() -> CompilerOptions + .versionToJdkLevel(project.getOption(JavaCore.COMPILER_COMPLIANCE, true)) >= ClassFileConstants.JDK9); if (java9plus) { this.knownModuleLocations = new HashMap<>();