[RFC][icedtea-web] remove redundant loading/storing of cache file was : Re: [RFC] Fix IndexOutOfBoundException because of corrupted entry in recently_used file

[Thomas Meyer]

Am Freitag, den 06.04.2012, 11:55 +0200 schrieb Jiri Vanek:
> 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.

Yes, you are right. this patch does the wrong thing.

Another idea to decrease number of reads:

I thought about storing the file timestamp of the last modification
after store() and compare this timestamp in the load() method, if the
timestamp differs we need to reload the file. if not we can skip the
load. what do you think about this idea?

> 
>  From my point of view I would rather stay without this fix - unless some more serious occurrences 
> will rise from this issue.
> 
> Ok?

okay.

> 
> Best regards,
>    J.