[RFC][icedtea-web] remove redundant loading/storing of cache file was : Re: [RFC] Fix IndexOutOfBoundException because of corrupted entry in recently_used file
Jiri Vanek
jvanek at redhat.com
Fri Apr 6 02:55:18 PDT 2012
On 04/05/2012 05:17 PM, Thomas Meyer wrote:
> Am Donnerstag, den 05.04.2012, 15:30 +0200 schrieb Thomas Meyer:
>>>> > > > --- a/netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java Wed Mar 28 12:08:10 2012 +0200
>>>> > > > +++ b/netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java Wed Apr 04 16:48:09 2012 +0200
>>>> > > > @@ -107,6 +107,49 @@
>>>> > > > */
>>>> > > > public synchronized void load() {
>>>> > > > cacheOrder.load();
>>>> > > > +
>>>> > > > + /*
>>>> > > > + * clean up possibly corrupted entries
>>>> > > > + */
>>> > > It is interesting to see how many times is checkData called during single jar run.. I counted
>>> > > 10executions.... But fixing this is definitely work for different patch.
>> >
>> > yeah, I also noticed this. maybe this can be reduced a bit. see below
>> > comment for getCacheFile(). but as the recently_used file could be
>> > corrupted by another jvm process (etc.), we need to check this on every
>> > load.
>> >
> maybe the attached patch helps here?
>
> with kind regards
> thomas
>
>
>
> reduce-number-of-loads-and-stores.patch
>
>
> diff -r 44a2eea54ac8 netx/net/sourceforge/jnlp/cache/CacheUtil.java
> --- a/netx/net/sourceforge/jnlp/cache/CacheUtil.java Wed Apr 04 16:01:21 2012 +0200
> +++ b/netx/net/sourceforge/jnlp/cache/CacheUtil.java Thu Apr 05 17:14:56 2012 +0200
> @@ -301,12 +301,18 @@
> synchronized (lruHandler) {
> lruHandler.lock();
>
> - // We need to reload the cacheOrder file each time
> - // since another plugin/javaws instance may have updated it.
> - lruHandler.load();
> cacheFile = getCacheFileIfExist(urlToPath(source, ""));
> + if(cacheFile == null) { // We did not find a copy of it.
> + // reload the cacheOrder file since another plugin/javaws instance may have updated it.
> + lruHandler.load();
> + cacheFile = getCacheFileIfExist(urlToPath(source, ""));
> + }
> +
> if (cacheFile == null) { // We did not find a copy of it.
> - cacheFile = makeNewCacheFile(source, version);
> + cacheFile = makeNewCacheFile(source, version, false);
> + } else {
> + // we did find and did update an entry, store it
> + lruHandler.store();
> }
> lruHandler.unlock();
> }
> @@ -360,10 +366,11 @@
> * @param version the version id of the local file
> * @return the file location in the cache.
> */
> - public static File makeNewCacheFile(URL source, Version version) {
> + public static File makeNewCacheFile(URL source, Version version, boolean load) {
> synchronized (lruHandler) {
> lruHandler.lock();
> - lruHandler.load();
> + if(load == true)
> + lruHandler.load();
>
> File cacheFile = null;
> for (long i = 0; i< Long.MAX_VALUE; i++) {
> diff -r 44a2eea54ac8 netx/net/sourceforge/jnlp/cache/ResourceTracker.java
> --- a/netx/net/sourceforge/jnlp/cache/ResourceTracker.java Wed Apr 04 16:01:21 2012 +0200
> +++ b/netx/net/sourceforge/jnlp/cache/ResourceTracker.java Thu Apr 05 17:14:56 2012 +0200
> @@ -786,7 +786,7 @@
> entry.markForDelete();
> entry.store();
> // Old entry will still exist. (but removed at cleanup)
> - localFile = CacheUtil.makeNewCacheFile(resource.location, resource.downloadVersion);
> + localFile = CacheUtil.makeNewCacheFile(resource.location, resource.downloadVersion, true);
> CacheEntry newEntry = new CacheEntry(resource.location, resource.requestVersion);
> newEntry.lock();
> entry.unlock();
>
Hmm. As I see it, this is removing some load() but is adding store(). I think that in case of
parallel javawss redundant reading does not hurt but more writing can hurt a lot.
From my point of view I would rather stay without this fix - unless some more serious occurrences
will rise from this issue.
Ok?
Best regards,
J.
More information about the distro-pkg-dev
mailing list