Bug 570896: Optimise happy path
By optimising the happy path of the lookup, we can save about 10% of
the cost of comparisons.
Refactor existing code into FileStoreUtil for convenience and to mark
other methods as private.
Change-Id: Ie828af3d31edfd032f7cdbb915f4757d516db0ab
Signed-off-by: Alex Blewitt <alex.blewitt@gmail.com>
diff --git a/bundles/org.eclipse.core.filesystem/src/org/eclipse/core/filesystem/IFileStore.java b/bundles/org.eclipse.core.filesystem/src/org/eclipse/core/filesystem/IFileStore.java
index 15f3397..0e5386f 100644
--- a/bundles/org.eclipse.core.filesystem/src/org/eclipse/core/filesystem/IFileStore.java
+++ b/bundles/org.eclipse.core.filesystem/src/org/eclipse/core/filesystem/IFileStore.java
@@ -577,25 +577,9 @@
* @since org.eclipse.core.filesystem 1.9
*/
public default int compareTo(IFileStore other) {
- int compare = FileStoreUtil.compareStringOrNull(this.getFileSystem().getScheme(), other.getFileSystem().getScheme());
- if (compare != 0)
- return compare;
- // compare based on URI path segment values
- URI uri1;
- URI uri2;
- try {
- uri1 = this.toURI();
- } catch (Exception e1) {
- // protect against misbehaving 3rd party code in file system implementations
- uri1 = null;
+ if (other == null) {
+ return 1;
}
- try {
- uri2 = other.toURI();
- } catch (Exception e2) {
- // protect against misbehaving 3rd party code in file system implementations
- uri2 = null;
- }
- // use old slow compare for compatibility reason. Does have a memory hotspot see bug 570896
- return FileStoreUtil.comparePathUri(uri1, uri2);
+ return FileStoreUtil.compareFileStore(this, other);
}
}
diff --git a/bundles/org.eclipse.core.filesystem/src/org/eclipse/core/internal/filesystem/FileStoreUtil.java b/bundles/org.eclipse.core.filesystem/src/org/eclipse/core/internal/filesystem/FileStoreUtil.java
index 0dbc549..4560d56 100644
--- a/bundles/org.eclipse.core.filesystem/src/org/eclipse/core/internal/filesystem/FileStoreUtil.java
+++ b/bundles/org.eclipse.core.filesystem/src/org/eclipse/core/internal/filesystem/FileStoreUtil.java
@@ -1,5 +1,5 @@
/*******************************************************************************
- * Copyright (c) 2021 Joerg Kubitz and others.
+ * Copyright (c) 2021 Joerg Kubitz and others.
*
* This program and the accompanying materials
* are made available under the terms of the Eclipse Public License 2.0
@@ -14,6 +14,7 @@
package org.eclipse.core.internal.filesystem;
import java.net.URI;
+import org.eclipse.core.filesystem.IFileStore;
import org.eclipse.core.runtime.IPath;
import org.eclipse.core.runtime.Path;
@@ -31,7 +32,7 @@
* This is the old slow implementation and has a memory hotspot see bug 570896. Prefer to use compareNormalisedUri!
* @since org.eclipse.core.filesystem 1.9
*/
- public static int comparePathUri(URI uri1, URI uri2) {
+ private static int comparePathUri(URI uri1, URI uri2) {
if (uri1 == null && uri2 == null)
return 0;
int compare;
@@ -110,7 +111,7 @@
return 0;
}
- static int comparePathSegments(String p1, String p2) {
+ public static int comparePathSegments(String p1, String p2) {
int compare;
compare = compareSlashFirst(p1, p2);
if (compare != 0)
@@ -159,7 +160,7 @@
* Compares two strings that are possibly null.
* @since org.eclipse.core.filesystem 1.9
*/
- public static int compareStringOrNull(String string1, String string2) {
+ private static int compareStringOrNull(String string1, String string2) {
if (string1 == null) {
if (string2 == null)
return 0;
@@ -169,4 +170,33 @@
return -1;
return string1.compareTo(string2);
}
+
+ /**
+ * Compares two file stores by comparing their URIs.
+ * @param fileStore1 the first fileStore to compare, cannot be null
+ * @param fileStore2 the second fileStore to compare, cannot be null
+ * @return 0 if the fileStores are equal, 1 if fileStore1 is bigger than fileStore2, -1 otherwise
+ */
+ public static int compareFileStore(IFileStore fileStore1, IFileStore fileStore2) {
+ int compare = compareStringOrNull(fileStore1.getFileSystem().getScheme(), fileStore2.getFileSystem().getScheme());
+ if (compare != 0)
+ return compare;
+ // compare based on URI path segment values
+ URI uri1;
+ URI uri2;
+ try {
+ uri1 = fileStore1.toURI();
+ } catch (Exception e1) {
+ // protect against misbehaving 3rd party code in file system implementations
+ uri1 = null;
+ }
+ try {
+ uri2 = fileStore2.toURI();
+ } catch (Exception e2) {
+ // protect against misbehaving 3rd party code in file system implementations
+ uri2 = null;
+ }
+ // use old slow compare for compatibility reason. Does have a memory hotspot see bug 570896
+ return comparePathUri(uri1, uri2);
+ }
}
diff --git a/bundles/org.eclipse.core.filesystem/src/org/eclipse/core/internal/filesystem/local/LocalFile.java b/bundles/org.eclipse.core.filesystem/src/org/eclipse/core/internal/filesystem/local/LocalFile.java
index dc68df5..d40504d 100644
--- a/bundles/org.eclipse.core.filesystem/src/org/eclipse/core/internal/filesystem/local/LocalFile.java
+++ b/bundles/org.eclipse.core.filesystem/src/org/eclipse/core/internal/filesystem/local/LocalFile.java
@@ -471,10 +471,12 @@
@Override
public int compareTo(IFileStore other) {
- int compare = FileStoreUtil.compareStringOrNull(this.getFileSystem().getScheme(), other.getFileSystem().getScheme());
- if (compare != 0)
- return compare;
- // override with fast implementation:
- return FileStoreUtil.compareNormalisedUri(this.toURI(), other.toURI());
+ if (other instanceof LocalFile) {
+ // We can compare paths in the local file implementation, because LocalFile don't have a query string, port, or authority
+ // We use `toURI` here because it performs file normalisation e.g. /a/b/../c -> /a/c
+ // The URI is cached by the LocalFile after normalisation so this effectively results in a straight lookup
+ return FileStoreUtil.comparePathSegments(this.toURI().getPath(), ((LocalFile) other).toURI().getPath());
+ }
+ return super.compareTo(other);
}
}
diff --git a/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/filesystem/FileStoreTest.java b/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/filesystem/FileStoreTest.java
index 28300e0..263edce 100755
--- a/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/filesystem/FileStoreTest.java
+++ b/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/filesystem/FileStoreTest.java
@@ -23,6 +23,7 @@
import org.eclipse.core.filesystem.*;
import org.eclipse.core.filesystem.provider.FileSystem;
import org.eclipse.core.internal.filesystem.NullFileSystem;
+import org.eclipse.core.internal.filesystem.local.LocalFile;
import org.eclipse.core.internal.filesystem.local.LocalFileSystem;
import org.eclipse.core.resources.IResource;
import org.eclipse.core.runtime.*;
@@ -649,6 +650,8 @@
assertEquals("3.1", schemeCompare, nabc.compareTo(labd));
assertEquals("3.2", -schemeCompare, labd.compareTo(nabc));
assertEquals("3.3", -schemeCompare, labc.compareTo(nabd));
+ assertEquals("4.0", 1, labc.compareTo(null));
+ assertEquals("4.1", 1, nabc.compareTo(null));
}
public void testSortOrderPaths() {
@@ -670,8 +673,15 @@
).collect(Collectors.toList());
paths = new ArrayList<>(paths); // to get a mutable copy for shuffling
Collections.shuffle(paths);
- Stream<IFileStore> stores = paths.stream().map(Path::new).map(lfs::getStore);
- List<String> sortedPaths = stores.sorted(IFileStore::compareTo).map(IFileStore::toURI).map(URI::getPath).collect(Collectors.toList());
- assertEquals("1.0 ", pathsTrimmed, sortedPaths);
+ // Test with new Path(string).getStore()
+ Stream<IFileStore> pathStores = paths.stream().map(Path::new).map(lfs::getStore);
+ List<String> sortedPathStores = pathStores.sorted(IFileStore::compareTo).map(IFileStore::toURI)
+ .map(URI::getPath).collect(Collectors.toList());
+ assertEquals("1.0 ", pathsTrimmed, sortedPathStores);
+ // Test with new LocalFile(new File(string)))
+ Stream<IFileStore> localFileStores = paths.stream().map(File::new).map(LocalFile::new);
+ List<String> sortedLocalFileStores = localFileStores.sorted(IFileStore::compareTo).map(IFileStore::toURI)
+ .map(URI::getPath).collect(Collectors.toList());
+ assertEquals("2.0 ", pathsTrimmed, sortedLocalFileStores);
}
}
