Bug 571340 - [performance] avoid validating Archives over and overI20210815-0600

- remember valid (!) archives (normal case).
- retry invalid archives
- synchronize directly on "invalidArchives" since that is final itself

Change-Id: I4f48d9b635dc5e4fe22938b98be03c2c45346281
Signed-off-by: Joerg Kubitz <jkubitz-eclipse@gmx.de>
Reviewed-on: https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/182540
Tested-by: JDT Bot <jdt-bot@eclipse.org>
Reviewed-by: Andrey Loskutov <loskutov@gmx.de>

1 files changed, 36 insertions, 43 deletions

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 cf98f9a415..b04bfffdb6 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
@@ -181,7 +181,7 @@ public class JavaModelManager implements ISaveParticipant, IContentTypeChangeLis
 	private static final String ASSUMED_EXTERNAL_FILES_CACHE = "assumedExternalFilesCache";  //$NON-NLS-1$
 
 	public static enum ArchiveValidity {
-		BAD_FORMAT, UNABLE_TO_READ, FILE_NOT_FOUND, VALID;
+		INVALID, VALID;
 
 		public boolean isValid() {
 			return this == VALID;
@@ -1329,7 +1329,7 @@ public class JavaModelManager implements ISaveParticipant, IContentTypeChangeLis
 		}
 
 		public synchronized ClasspathChange resetResolvedClasspath() {
-			// clear non-chaining jars cache and invalid jars cache
+			// clear non-chaining jars cache and external jars cache
 			JavaModelManager.getJavaModelManager().resetClasspathListCache();
 
 			// null out resolved information
@@ -1621,11 +1621,10 @@ public class JavaModelManager implements ISaveParticipant, IContentTypeChangeLis
 	}
 
 	/*
-	 * A map of IPaths for jars that are known to be invalid (such as not being in a valid/known format), to an eviction timestamp.
-	 * Synchronize on invalidArchivesMutex before accessing.
+	 * A map of IPaths for jars with known validity (such as being in a valid/known format or not), to an eviction timestamp.
+	 * Synchronize on invalidArchives before accessing.
 	 */
 	private final Map<IPath, InvalidArchiveInfo> invalidArchives = new HashMap<>();
-	private final Object invalidArchivesMutex = new Object();
 
 	/*
 	 * A set of IPaths for files that are known to be external to the workspace.
@@ -1797,9 +1796,9 @@ public class JavaModelManager implements ISaveParticipant, IContentTypeChangeLis
 
 	public void addInvalidArchive(IPath path, ArchiveValidity reason) {
 		if (DEBUG_INVALID_ARCHIVES) {
-			System.out.println("Invalid JAR cache: adding " + path + ", reason: " + reason);  //$NON-NLS-1$//$NON-NLS-2$
+			System.out.println("JAR cache: adding " + reason + " " + path);  //$NON-NLS-1$//$NON-NLS-2$
 		}
-		synchronized (this.invalidArchivesMutex) {
+		synchronized (this.invalidArchives) {
 			this.invalidArchives.put(path, new InvalidArchiveInfo(System.currentTimeMillis() + INVALID_ARCHIVE_TTL_MILLISECONDS, reason));
 		}
 	}
@@ -2841,7 +2840,9 @@ public class JavaModelManager implements ISaveParticipant, IContentTypeChangeLis
 		if (isJrt(path)) {
 			return;
 		}
-		throwExceptionIfArchiveInvalid(path);
+		if (isArchiveStateKnownToBeValid(path)) {
+			return; // known to be valid
+		}
 		ZipFile file = getZipFile(path);
 		closeZipFile(file);
 	}
@@ -2875,7 +2876,7 @@ public class JavaModelManager implements ISaveParticipant, IContentTypeChangeLis
 
 	public ZipFile getZipFile(IPath path, boolean checkInvalidArchiveCache) throws CoreException {
 		if (checkInvalidArchiveCache) {
-			throwExceptionIfArchiveInvalid(path);
+			isArchiveStateKnownToBeValid(path);
 		}
 		ZipCache zipCache;
 		ZipFile zipFile;
@@ -2896,17 +2897,11 @@ public class JavaModelManager implements ISaveParticipant, IContentTypeChangeLis
 			if (zipCache != null) {
 				zipCache.setCache(path, zipFile);
 			}
+			addInvalidArchive(path, ArchiveValidity.VALID); // remember its valid
 			return zipFile;
 		} catch (IOException e) {
-			ArchiveValidity reason;
-
-			if (e instanceof ZipException) {
-				reason = ArchiveValidity.BAD_FORMAT;
-			} else if (e instanceof FileNotFoundException) {
-				reason = ArchiveValidity.FILE_NOT_FOUND;
-			} else {
-				reason = ArchiveValidity.UNABLE_TO_READ;
-			}
+			// file may exist but for some reason is inaccessible
+			ArchiveValidity reason=ArchiveValidity.INVALID;
 			addInvalidArchive(path, reason);
 			throw new CoreException(new Status(IStatus.ERROR, JavaCore.PLUGIN_ID, -1, Messages.status_IOException, e));
 		}
@@ -2932,18 +2927,12 @@ public class JavaModelManager implements ISaveParticipant, IContentTypeChangeLis
 		return localFile;
 	}
 
-	private void throwExceptionIfArchiveInvalid(IPath path) throws CoreException {
+	private boolean isArchiveStateKnownToBeValid(IPath path) throws CoreException {
 		ArchiveValidity validity = getArchiveValidity(path);
-		IOException reason;
-		switch (validity) {
-			case BAD_FORMAT: reason = new ZipException("Bad format in archive: " + path); break; //$NON-NLS-1$
-			case FILE_NOT_FOUND: reason = new FileNotFoundException("Archive not found for path: " + path); break; //$NON-NLS-1$
-			case UNABLE_TO_READ: reason = new IOException("Unable to read archive: " + path); break; //$NON-NLS-1$
-			default: reason = null;
-		}
-		if (reason != null) {
-			throw new CoreException(new Status(IStatus.ERROR, JavaCore.PLUGIN_ID, -1, Messages.status_IOException, reason));
+		if (validity == null || validity == ArchiveValidity.INVALID) {
+			return false; // chance the file has become accessible/readable now.
 		}
+		return true;
 	}
 
 	/*
@@ -3399,11 +3388,15 @@ public class JavaModelManager implements ISaveParticipant, IContentTypeChangeLis
 
 	public ArchiveValidity getArchiveValidity(IPath path) {
 		InvalidArchiveInfo invalidArchiveInfo;
-		synchronized (this.invalidArchivesMutex) {
+		synchronized (this.invalidArchives) {
 			invalidArchiveInfo = this.invalidArchives.get(path);
 		}
-		if (invalidArchiveInfo == null)
-			return ArchiveValidity.VALID;
+		if (invalidArchiveInfo == null) {
+			if (DEBUG_INVALID_ARCHIVES) {
+				System.out.println("JAR cache: UNKNOWN validity for " + path);  //$NON-NLS-1$
+			}
+			return null;
+		}
 		long now = System.currentTimeMillis();
 
 		// If the TTL for this cache entry has expired, directly check whether the archive is still invalid.
@@ -3418,14 +3411,24 @@ public class JavaModelManager implements ISaveParticipant, IContentTypeChangeLis
 			// Retry the test from the start, now that we have an up-to-date result
 			return getArchiveValidity(path);
 		}
+		if (DEBUG_INVALID_ARCHIVES) {
+			System.out.println("JAR cache: " + invalidArchiveInfo.reason + " " + path);  //$NON-NLS-1$ //$NON-NLS-2$
+		}
 		return invalidArchiveInfo.reason;
 	}
 
 	public void removeFromInvalidArchiveCache(IPath path) {
-		synchronized(this.invalidArchivesMutex) {
+		synchronized(this.invalidArchives) {
+			InvalidArchiveInfo entry = this.invalidArchives.get(path);
+			if (entry != null && entry.reason == ArchiveValidity.VALID) {
+				if (DEBUG_INVALID_ARCHIVES) {
+					System.out.println("JAR cache: keep VALID " + path);  //$NON-NLS-1$
+				}
+				return; // do not remove the VALID information
+			}
 			if (this.invalidArchives.remove(path) != null) {
 				if (DEBUG_INVALID_ARCHIVES) {
-					System.out.println("Invalid JAR cache: removed " + path);  //$NON-NLS-1$
+					System.out.println("JAR cache: removed INVALID " + path);  //$NON-NLS-1$
 				}
 				try {
 					// Bug 455042: Force an update of the JavaProjectElementInfo project caches.
@@ -4292,16 +4295,6 @@ public class JavaModelManager implements ISaveParticipant, IContentTypeChangeLis
 	public void resetClasspathListCache() {
 		if (this.nonChainingJars != null)
 			this.nonChainingJars.clear();
-		if (DEBUG_INVALID_ARCHIVES) {
-			synchronized(this.invalidArchivesMutex) {
-				if (!this.invalidArchives.isEmpty()) {
-					System.out.println("Invalid JAR cache: clearing cache"); //$NON-NLS-1$
-				}
-			}
-		}
-		synchronized(this.invalidArchivesMutex) {
-			this.invalidArchives.clear();
-		}
 		if (this.externalFiles != null)
 			this.externalFiles.clear();
 		if (this.assumedExternalFiles != null)