MavenSupport: Add cache for remote artifacts #122
Conversation
|
Could you elaborate in the commit message why this makes sense? Naively thinking I would have assumed Maven artifacts get already cached by Maven itself in the standard local Maven directory. And even if they would not get cached implicitly, I would have assumed "turning on" the feature to cache in Maven-Local is less work than implementing an own cache with DiskLruCache. |
| @@ -66,7 +69,12 @@ fun Artifact.identifier() = "$groupId:$artifactId:$version" | |||
|
|
|||
| class MavenSupport(localRepositoryManagerConverter: (LocalRepositoryManager) -> LocalRepositoryManager) { | |||
| companion object { | |||
| private const val GIGABYTE = 1073741824L | |||
There was a problem hiding this comment.
Nit: Write as 1024L * 1024L?
| @@ -125,6 +133,16 @@ class MavenSupport(localRepositoryManagerConverter: (LocalRepositoryManager) -> | |||
| } | |||
|
|
|||
| fun requestRemoteArtifact(artifact: Artifact, repositories: List<RemoteRepository>): RemoteArtifact { | |||
| val cacheKey = artifact.toString() | |||
There was a problem hiding this comment.
Use String.fileSystemEncode() instead?
There was a problem hiding this comment.
The DiskLruCache only allows keys matching "[a-z0-9_-]{1,64}" which is stricter than fileSystemEncode().
util/src/main/kotlin/DiskCache.kt
Outdated
|
|
||
| package com.here.ort.util | ||
|
|
||
| import ch.frankel.slf4k.error |
There was a problem hiding this comment.
Nit: Always use wildcard import for this package.
| val timeToLive: Int | ||
| ) { | ||
| companion object { | ||
| const val INDEX_FULL_KEY = 0 |
There was a problem hiding this comment.
How about making this an INDEX enum and using INDEX.values().size instead of VALUE_COUNT?
There was a problem hiding this comment.
I gave it a try, but my impression was it adds more overhead than benefit.
util/src/main/kotlin/DiskCache.kt
Outdated
|
|
||
| /** | ||
| * Shorten the string to be usable as a key for [DiskLruCache] which has a maximum length of 64. If the string is | ||
| * shorter than 57 chars take it as is, otherwise shorten it and append a serial number. The un-shortened string is |
There was a problem hiding this comment.
Improved the documentation.
util/src/main/kotlin/DiskCache.kt
Outdated
| * stored in the cache to be able to compare if a cache entry matches a key. | ||
| */ | ||
| private fun String.asKey(): String { | ||
| return if (length > 57) { |
There was a problem hiding this comment.
Extract 57 to a local variable / constant?
util/src/main/kotlin/DiskCache.kt
Outdated
| "Cannot generate key for '$this' because all possible keys starting with '$key' are taken.") | ||
| } else { | ||
| // String is short enough to be unique, use it as is. | ||
| this |
There was a problem hiding this comment.
Maybe make the easy case go first and return early, and only use the tryKey logic as a fallback lateron?
util/src/main/kotlin/DiskCache.kt
Outdated
| private val diskLruCache = DiskLruCache.open(directory, 0, VALUE_COUNT, maxSize) | ||
|
|
||
| /** | ||
| * Shorten the string to be usable as a key for [DiskLruCache] which has a maximum length of 64. If the string is |
There was a problem hiding this comment.
Looks like the limit is 120 now: JakeWharton/DiskLruCache@057db12#diff-04c6e90faac2675aa89e2176d2eec7d8
There was a problem hiding this comment.
Unfortunately, there's no official release with that change. We could use JitPack to dynamically create one, or fork the project an publish our own release.
There was a problem hiding this comment.
We would still need to implement the same logic, as keys can also be longer than 120 chars, so I think we wouldn't win much.
There is also JakeWharton/DiskLruCache#98, but it seems Jake is not active on this project anymore.
mnonnenmacher
force-pushed
the
maven-caching
branch
from
dc31bcc
to
d961b78
Compare
util/src/main/kotlin/DiskCache.kt
Outdated
| this | ||
| } else { | ||
| // String is too long to be unique, append it with a serial number. | ||
| val key = substring(0..minOf(57, length - 1)) |
util/src/main/kotlin/DiskCache.kt
Outdated
| } else { | ||
| // String is too long to be unique, append it with a serial number. | ||
| val key = substring(0..minOf(57, length - 1)) | ||
| for (index in 0..99999) { |
There was a problem hiding this comment.
The length of the upper bound number should be aligned to KEY_SUFFIX_LENGTH.
There was a problem hiding this comment.
Done. Changed the constant to 6.
mnonnenmacher
force-pushed
the
maven-caching
branch
from
d961b78
to
6c26437
Compare
Store remote artifact information in a disk cache backed by DiskLruCache [1]. This brings a big performance boost for Gradle, Maven and SBT dependency resolution. The cached metadata contains the URLs and checksums of Maven artifacts which is retrieved without actually downloading the remote artifacts. This is information which is not available from the local Maven repository, especially without downloading the remote artifacts. Currently the max cache size is hardcoded to 1GB and expiry to six hours, this will be made configurable in a later commit. [1] https://github.com/JakeWharton/DiskLruCache
mnonnenmacher
force-pushed
the
maven-caching
branch
from
6c26437
to
84a71d3
Compare
Fix: Allow custom config also for scanner
Store remote artifact information in a disk cache backed by
DiskLruCache [1]. This brings a big performance boost for Gradle, Maven
and SBT dependency resolution.
The cached metadata contains the URLs and checksums of Maven artifacts which
is retrieved without actually downloading the remote artifacts. This is
information which is not available from the local Maven repository,
especially without downloading the remote artifacts.
Currently the max cache size is hardcoded to 1GB and expiry to six hours,
this will be made configurable in a later commit.
[1] https://github.com/JakeWharton/DiskLruCache
This change is