From a40d7ba1f0b7935e537287c89ef631d2a305e156 Mon Sep 17 00:00:00 2001 From: BJ Hargrave Date: Fri, 23 Sep 2022 13:59:40 -0400 Subject: [PATCH 1/4] m2e: Add a toString method override for debugging Signed-off-by: BJ Hargrave --- bndtools.m2e/src/bndtools/m2e/MavenWorkspaceRepository.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/bndtools.m2e/src/bndtools/m2e/MavenWorkspaceRepository.java b/bndtools.m2e/src/bndtools/m2e/MavenWorkspaceRepository.java index 9b749c2ac0..f2480aa19b 100644 --- a/bndtools.m2e/src/bndtools/m2e/MavenWorkspaceRepository.java +++ b/bndtools.m2e/src/bndtools/m2e/MavenWorkspaceRepository.java @@ -205,6 +205,11 @@ public String getName() { return "Maven Workspace"; } + @Override + public String toString() { + return getName(); + } + @Override public boolean isEmpty() { return list.get() From 4533cdf472d3dc723ed20ac0e6cef97ea1792dd5 Mon Sep 17 00:00:00 2001 From: BJ Hargrave Date: Fri, 23 Sep 2022 14:01:02 -0400 Subject: [PATCH 2/4] resource: Add addResource method to ResourceBuilder Signed-off-by: BJ Hargrave --- .../aQute/bnd/osgi/resource/ResourceBuilder.java | 15 +++++++++++++-- .../src/aQute/bnd/osgi/resource/package-info.java | 2 +- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/biz.aQute.bndlib/src/aQute/bnd/osgi/resource/ResourceBuilder.java b/biz.aQute.bndlib/src/aQute/bnd/osgi/resource/ResourceBuilder.java index 412b119f94..558c3753ab 100644 --- a/biz.aQute.bndlib/src/aQute/bnd/osgi/resource/ResourceBuilder.java +++ b/biz.aQute.bndlib/src/aQute/bnd/osgi/resource/ResourceBuilder.java @@ -73,13 +73,19 @@ public class ResourceBuilder { private boolean built = false; + public ResourceBuilder() {} + public ResourceBuilder(Resource source) { + this(); + addResource(source); + } + + public ResourceBuilder addResource(Resource source) { addCapabilities(source.getCapabilities(null)); addRequirements(source.getRequirements(null)); + return this; } - public ResourceBuilder() {} - public ResourceBuilder addCapability(Capability capability) { CapReqBuilder builder = CapReqBuilder.clone(capability); return addCapability(builder); @@ -833,6 +839,11 @@ public Resource build() { return null; } + @Override + public ResourceBuilder addResource(Resource source) { + return ResourceBuilder.this.addResource(source); + } + @Override public ResourceBuilder addCapability(Capability capability) { return ResourceBuilder.this.addCapability(capability); diff --git a/biz.aQute.bndlib/src/aQute/bnd/osgi/resource/package-info.java b/biz.aQute.bndlib/src/aQute/bnd/osgi/resource/package-info.java index 3a825f3eda..b12cd2b413 100644 --- a/biz.aQute.bndlib/src/aQute/bnd/osgi/resource/package-info.java +++ b/biz.aQute.bndlib/src/aQute/bnd/osgi/resource/package-info.java @@ -1,4 +1,4 @@ -@Version("4.2.0") +@Version("4.3.0") package aQute.bnd.osgi.resource; import org.osgi.annotation.versioning.Version; From e140700175d257ec854c5d13b6f081397b9425e7 Mon Sep 17 00:00:00 2001 From: BJ Hargrave Date: Mon, 26 Sep 2022 10:03:05 -0400 Subject: [PATCH 3/4] deferred value: Memoize value to ensure at-most-once supplier call Since we are using deferred values to defer expensive operations, we don't want concurrent threads all performing the expensive operation. So we memoize the result of the operation to ensure at-most-once. Signed-off-by: BJ Hargrave --- .../src/aQute/bnd/osgi/resource/DeferredValue.java | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/biz.aQute.bndlib/src/aQute/bnd/osgi/resource/DeferredValue.java b/biz.aQute.bndlib/src/aQute/bnd/osgi/resource/DeferredValue.java index 9ba943d5de..3fba737685 100644 --- a/biz.aQute.bndlib/src/aQute/bnd/osgi/resource/DeferredValue.java +++ b/biz.aQute.bndlib/src/aQute/bnd/osgi/resource/DeferredValue.java @@ -4,25 +4,22 @@ import java.util.function.Supplier; +import aQute.bnd.memoize.Memoize; + class DeferredValue implements Supplier { private final Class type; private final Supplier supplier; private final int hashCode; - private T value; DeferredValue(Class type, Supplier supplier, int hashCode) { this.type = requireNonNull(type); - this.supplier = requireNonNull(supplier); + this.supplier = Memoize.supplier(supplier); this.hashCode = hashCode; } @Override public T get() { - T v = value; - if (v == null) { - return value = supplier.get(); - } - return v; + return supplier.get(); } Class type() { From 7b42679baf2ad90a3218e14aa0e07b4f8264c8e0 Mon Sep 17 00:00:00 2001 From: BJ Hargrave Date: Fri, 23 Sep 2022 14:50:32 -0400 Subject: [PATCH 4/4] resource: Add a File Resource cache to ResourceBuilder The cache reduces the need to process files to Resource objects, including SHA-256 computation, for unchanged files. Fixes https://github.com/bndtools/bnd/issues/5367 Signed-off-by: BJ Hargrave --- .../resource/FileResourceCacheKeyTest.java | 127 ++++++++++++++++ .../bnd/osgi/resource/FileResourceCache.java | 136 ++++++++++++++++++ .../bnd/osgi/resource/ResourceBuilder.java | 27 +--- .../fileset/FileSetRepositoryTest.java | 11 ++ 4 files changed, 280 insertions(+), 21 deletions(-) create mode 100644 biz.aQute.bndlib.tests/test/aQute/bnd/osgi/resource/FileResourceCacheKeyTest.java create mode 100644 biz.aQute.bndlib/src/aQute/bnd/osgi/resource/FileResourceCache.java diff --git a/biz.aQute.bndlib.tests/test/aQute/bnd/osgi/resource/FileResourceCacheKeyTest.java b/biz.aQute.bndlib.tests/test/aQute/bnd/osgi/resource/FileResourceCacheKeyTest.java new file mode 100644 index 0000000000..841d139138 --- /dev/null +++ b/biz.aQute.bndlib.tests/test/aQute/bnd/osgi/resource/FileResourceCacheKeyTest.java @@ -0,0 +1,127 @@ +package aQute.bnd.osgi.resource; + +import static org.assertj.core.api.Assertions.assertThatObject; + +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.attribute.BasicFileAttributeView; +import java.nio.file.attribute.BasicFileAttributes; +import java.nio.file.attribute.FileTime; +import java.time.Instant; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledOnOs; +import org.junit.jupiter.api.condition.OS; + +import aQute.bnd.osgi.resource.FileResourceCache.CacheKey; +import aQute.bnd.test.jupiter.InjectTemporaryDirectory; +import aQute.lib.io.IO; + +class FileResourceCacheKeyTest { + + @Test + void unchanged(@InjectTemporaryDirectory + Path tmp) throws Exception { + Path subject = tmp.resolve("test"); + IO.store("line1", subject, StandardCharsets.UTF_8); + CacheKey key1 = new CacheKey(subject); + CacheKey key2 = new CacheKey(subject); + assertThatObject(key1).isEqualTo(key2); + assertThatObject(key1).hasSameHashCodeAs(key2); + } + + @Test + void change_modified(@InjectTemporaryDirectory + Path tmp) throws Exception { + Path subject = tmp.resolve("test"); + IO.store("line1", subject, StandardCharsets.UTF_8); + CacheKey key1 = new CacheKey(subject); + BasicFileAttributes attributes = Files.getFileAttributeView(subject, BasicFileAttributeView.class) + .readAttributes(); + FileTime lastModifiedTime = attributes.lastModifiedTime(); + Instant plusSeconds = lastModifiedTime.toInstant() + .plusSeconds(10L); + Files.setLastModifiedTime(subject, FileTime.from(plusSeconds)); + CacheKey key2 = new CacheKey(subject); + assertThatObject(key1).isNotEqualTo(key2); + assertThatObject(key1).doesNotHaveSameHashCodeAs(key2); + } + + @Test + void change_size(@InjectTemporaryDirectory + Path tmp) throws Exception { + Path subject = tmp.resolve("test"); + IO.store("line1", subject, StandardCharsets.UTF_8); + CacheKey key1 = new CacheKey(subject); + BasicFileAttributes attributes = Files.getFileAttributeView(subject, BasicFileAttributeView.class) + .readAttributes(); + FileTime lastModifiedTime = attributes.lastModifiedTime(); + IO.store("line100", subject, StandardCharsets.UTF_8); + Files.setLastModifiedTime(subject, lastModifiedTime); + CacheKey key2 = new CacheKey(subject); + assertThatObject(key1).isNotEqualTo(key2); + assertThatObject(key1).doesNotHaveSameHashCodeAs(key2); + } + + @DisabledOnOs(value = OS.WINDOWS, disabledReason = "Windows FS does not support fileKey") + @Test + void change_filekey(@InjectTemporaryDirectory + Path tmp) throws Exception { + Path subject = tmp.resolve("test"); + IO.store("line1", subject, StandardCharsets.UTF_8); + CacheKey key1 = new CacheKey(subject); + BasicFileAttributes attributes = Files.getFileAttributeView(subject, BasicFileAttributeView.class) + .readAttributes(); + assertThatObject(attributes.fileKey()).isNotNull(); + FileTime lastModifiedTime = attributes.lastModifiedTime(); + Path subject2 = tmp.resolve("test.tmp"); + IO.store("line2", subject2, StandardCharsets.UTF_8); + Files.setLastModifiedTime(subject2, lastModifiedTime); + IO.rename(subject2, subject); + CacheKey key2 = new CacheKey(subject); + attributes = Files.getFileAttributeView(subject, BasicFileAttributeView.class) + .readAttributes(); + assertThatObject(attributes.fileKey()).isNotNull(); + assertThatObject(key1).as("key2 not equal") + .isNotEqualTo(key2); + assertThatObject(key1).as("key2 different hash") + .doesNotHaveSameHashCodeAs(key2); + } + + @Test + void change_file_modified(@InjectTemporaryDirectory + Path tmp) throws Exception { + Path subject = tmp.resolve("test"); + IO.store("line1", subject, StandardCharsets.UTF_8); + CacheKey key1 = new CacheKey(subject); + Path subject2 = tmp.resolve("test.tmp"); + IO.store("line2", subject2, StandardCharsets.UTF_8); + BasicFileAttributes attributes = Files.getFileAttributeView(subject2, BasicFileAttributeView.class) + .readAttributes(); + FileTime lastModifiedTime = attributes.lastModifiedTime(); + Instant plusSeconds = lastModifiedTime.toInstant() + .plusSeconds(10L); + Files.setLastModifiedTime(subject2, FileTime.from(plusSeconds)); + IO.rename(subject2, subject); + CacheKey key2 = new CacheKey(subject); + assertThatObject(key1).as("key2 not equal") + .isNotEqualTo(key2); + assertThatObject(key1).as("key2 different hash") + .doesNotHaveSameHashCodeAs(key2); + } + + @Test + void different_files(@InjectTemporaryDirectory + Path tmp) throws Exception { + Path subject1 = tmp.resolve("test1"); + IO.store("line1", subject1, StandardCharsets.UTF_8); + CacheKey key1 = new CacheKey(subject1); + Path subject2 = tmp.resolve("test2"); + IO.copy(subject1, subject2); + CacheKey key2 = new CacheKey(subject2); + assertThatObject(key1).isNotEqualTo(key2); + assertThatObject(key1).doesNotHaveSameHashCodeAs(key2); + } + +} diff --git a/biz.aQute.bndlib/src/aQute/bnd/osgi/resource/FileResourceCache.java b/biz.aQute.bndlib/src/aQute/bnd/osgi/resource/FileResourceCache.java new file mode 100644 index 0000000000..9bfe5e79a9 --- /dev/null +++ b/biz.aQute.bndlib/src/aQute/bnd/osgi/resource/FileResourceCache.java @@ -0,0 +1,136 @@ +package aQute.bnd.osgi.resource; + +import static aQute.bnd.exceptions.SupplierWithException.asSupplierOrElse; +import static aQute.bnd.osgi.Constants.MIME_TYPE_BUNDLE; +import static aQute.bnd.osgi.Constants.MIME_TYPE_JAR; + +import java.io.File; +import java.io.IOException; +import java.net.URI; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.attribute.BasicFileAttributeView; +import java.nio.file.attribute.BasicFileAttributes; +import java.util.Map; +import java.util.Objects; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.TimeUnit; + +import org.osgi.resource.Resource; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import aQute.bnd.exceptions.Exceptions; +import aQute.bnd.osgi.Domain; +import aQute.libg.cryptography.SHA256; + +class FileResourceCache { + private final static Logger logger = LoggerFactory.getLogger(FileResourceCache.class); + private final static long EXPIRED_DURATION_NANOS = TimeUnit.NANOSECONDS.convert(30L, + TimeUnit.MINUTES); + private static final FileResourceCache INSTANCE = new FileResourceCache(); + private final Map cache; + private long time; + + private FileResourceCache() { + cache = new ConcurrentHashMap<>(); + time = System.nanoTime(); + } + + static FileResourceCache getInstance() { + return INSTANCE; + } + + Resource getResource(File file, URI uri) { + if (!file.isFile()) { + return null; + } + // Make sure we don't grow infinitely + final long now = System.nanoTime(); + if ((now - time) > EXPIRED_DURATION_NANOS) { + time = now; + cache.keySet() + .removeIf(key -> (now - key.time) > EXPIRED_DURATION_NANOS); + } + CacheKey cacheKey = new CacheKey(file); + Resource resource = cache.computeIfAbsent(cacheKey, key -> { + logger.debug("parsing {}", file); + ResourceBuilder rb = new ResourceBuilder(); + try { + Domain manifest = Domain.domain(file); + boolean hasIdentity = false; + if (manifest != null) { + hasIdentity = rb.addManifest(manifest); + } + String mime = hasIdentity ? MIME_TYPE_BUNDLE : MIME_TYPE_JAR; + DeferredValue sha256 = new DeferredComparableValue<>(String.class, + asSupplierOrElse(() -> SHA256.digest(file) + .asHex(), null), + key.hashCode()); + rb.addContentCapability(uri, sha256, file.length(), mime); + + if (hasIdentity) { + rb.addHashes(file); + } + } catch (Exception e) { + throw Exceptions.duck(e); + } + return rb.build(); + }); + return resource; + } + + static final class CacheKey { + private final Object fileKey; + private final long lastModifiedTime; + private final long size; + final long time; + + CacheKey(File file) { + this(file.toPath()); + } + + CacheKey(Path path) { + BasicFileAttributes attributes; + try { + attributes = Files.getFileAttributeView(path, BasicFileAttributeView.class) + .readAttributes(); + } catch (IOException e) { + throw Exceptions.duck(e); + } + if (!attributes.isRegularFile()) { + throw new IllegalArgumentException("File must be a regular file: " + path); + } + Object fileKey = attributes.fileKey(); + this.fileKey = (fileKey != null) ? fileKey // + : path.toAbsolutePath(); // Windows FS does not have fileKey + this.lastModifiedTime = attributes.lastModifiedTime() + .toMillis(); + this.size = attributes.size(); + this.time = System.nanoTime(); + } + + @Override + public int hashCode() { + return (Objects.hashCode(fileKey) * 31 + Long.hashCode(lastModifiedTime)) * 31 + Long.hashCode(size); + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (!(obj instanceof CacheKey)) { + return false; + } + CacheKey other = (CacheKey) obj; + return Objects.equals(fileKey, other.fileKey) && (lastModifiedTime == other.lastModifiedTime) + && (size == other.size); + } + + @Override + public String toString() { + return Objects.toString(fileKey); + } + } +} diff --git a/biz.aQute.bndlib/src/aQute/bnd/osgi/resource/ResourceBuilder.java b/biz.aQute.bndlib/src/aQute/bnd/osgi/resource/ResourceBuilder.java index 558c3753ab..17ca1ffdf8 100644 --- a/biz.aQute.bndlib/src/aQute/bnd/osgi/resource/ResourceBuilder.java +++ b/biz.aQute.bndlib/src/aQute/bnd/osgi/resource/ResourceBuilder.java @@ -1,6 +1,5 @@ package aQute.bnd.osgi.resource; -import static aQute.bnd.exceptions.SupplierWithException.asSupplierOrElse; import static aQute.bnd.osgi.Constants.DUPLICATE_MARKER; import static aQute.bnd.osgi.Constants.MIME_TYPE_BUNDLE; import static aQute.bnd.osgi.Constants.MIME_TYPE_JAR; @@ -61,7 +60,6 @@ import aQute.lib.hierarchy.Hierarchy; import aQute.lib.hierarchy.NamedNode; import aQute.lib.zip.JarIndex; -import aQute.libg.cryptography.SHA256; import aQute.libg.reporter.ReporterAdapter; import aQute.service.reporter.Reporter; @@ -730,30 +728,17 @@ public boolean addFile(File file, URI uri) throws Exception { if (uri == null) uri = file.toURI(); - Domain manifest = Domain.domain(file); boolean hasIdentity = false; - if (manifest != null) { - hasIdentity = addManifest(manifest); - } - String mime = hasIdentity ? MIME_TYPE_BUNDLE : MIME_TYPE_JAR; - int deferredHashCode = hashCode(file); - DeferredValue sha256 = new DeferredComparableValue<>(String.class, - asSupplierOrElse(() -> SHA256.digest(file) - .asHex(), null), - deferredHashCode); - addContentCapability(uri, sha256, file.length(), mime); - - if (hasIdentity) { - addHashes(file); + Resource fileResource = FileResourceCache.getInstance() + .getResource(file, uri); + if (fileResource != null) { + addResource(fileResource); + hasIdentity = !fileResource.getCapabilities(IdentityNamespace.IDENTITY_NAMESPACE) + .isEmpty(); } return hasIdentity; } - private static int hashCode(File file) { - return file.getAbsoluteFile() - .hashCode(); - } - /** * Add simple class name hashes to the exported packages. This should not be * called before any package capabilities are set since we only hash class diff --git a/biz.aQute.repository/test/aQute/bnd/repository/fileset/FileSetRepositoryTest.java b/biz.aQute.repository/test/aQute/bnd/repository/fileset/FileSetRepositoryTest.java index 8fe12f74c3..54d811085b 100644 --- a/biz.aQute.repository/test/aQute/bnd/repository/fileset/FileSetRepositoryTest.java +++ b/biz.aQute.repository/test/aQute/bnd/repository/fileset/FileSetRepositoryTest.java @@ -30,6 +30,17 @@ public void includesMavenArtifacts() throws Exception { assertThat(repository.list(null)).contains("org.nanohttpd:nanohttpd", "javafx.base") .doesNotContain("javax.annotation:jsr250-api"); + // Do it again which will get file resources from the cache + repository = new FileSetRepository("test2", files); + + assertThat(repository.list(null)).contains("org.nanohttpd:nanohttpd", "javafx.base") + .doesNotContain("javax.annotation:jsr250-api"); + + assertThat(repository.refresh()).isTrue(); + + assertThat(repository.list(null)).contains("org.nanohttpd:nanohttpd", "javafx.base") + .doesNotContain("javax.annotation:jsr250-api"); + } }