[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