[RFC][Icedtea-web]: Changed how cache works.

[Andrew Su]

----- Original Message -----
> From: "Omair Majid" <omajid at redhat.com>
> To: "Andrew Su" <asu at redhat.com>
> Cc: "Denis Lila" <dlila at redhat.com>, "IcedTea" <distro-pkg-dev at openjdk.java.net>
> Sent: Friday, April 15, 2011 4:48:30 PM
> Subject: Re: [RFC][Icedtea-web]: Changed how cache works.
> Hey,
> 
> While the patch looks fine overall, there are a few changes that I
> think
> would be good to have. It's really your call if you want to commit the
> patch first and then fix them or fix them first.
> 
> > + /* back-end of how LRU is implemented */
> > + private PropertiesFile cacheOrder = new PropertiesFile(new
> > File(cacheDir + File.separator + "RecentlyUsed"));
> > +
> 
> Andrew Hughes posted that he would have preferred a name like
> recently_used instead of RecentlyUsed. Can you please fix this?

Ah yes, this was a part of some changes I did, but forgot to hg add (then forgot to tell Denis when he change file name for me). Fixed.

> 
> > + public synchronized boolean addEntry(String identifier, String
> > path) {
> 
> > + public synchronized boolean removeEntry(String key) {
> 
> > + public synchronized boolean updateEntry(String oldKey) {
> 
> I would have been happier with all these methods taking identifiers or
> keys. As such, there is on way to add an entry and then remove it (as
> you wouldnt know the key for it).

I've changed it to take a key for addEntry instead of identifier.

> 
> > + @SuppressWarnings("unchecked")
> > + public synchronized List<Entry<String, String>>
> > getLRUSortedEntries() {
> 
> Is there a reason to suppress these warnings?

Removed

> 
> > + /*
> > + * This file is to keep track of the most recently used items.
> > + * The items are to be kept with
> > + * key = (current time accessed) followed by folder of item.
> > + * value = path to file.
> > + */
> > +
> > + private static final CacheLRUWrapper lruHandler =
> > CacheLRUWrapper.getInstance();
> 
> The comment describes an internal representation which is now
> encapsulated in CacheLRUWrapper. I am also not sure if it is accurate.
> Please (re)move it.

Moved to CacheLRUWrapper.

> 
> >
> > + public static void cleanCache() {
> > + if (okToClearCache()) {
> > + // First we want to figure out which stuff we need to delete.
> > + // There should not be any duplicate entries in cacheOrder, since
> > we always clear old when setting new.
> > + // Thus we don't need to search through that.
> > + // We now go through all the entries and pick out the ones that
> > aren't marked for delete starting from the back.
> > + HashSet<String> keep = new HashSet<String>();
> 
> is 'keep' used right now?

Yup, it's being used to help us remove redundant entries.

> 
> > @@ -754,6 +779,17 @@
> >
> >               int size = connection.getContentLength();
> >               boolean current =
> >               CacheUtil.isCurrent(resource.location,
> >               resource.requestVersion, connection)&&
> >               resource.getUpdatePolicy() != UpdatePolicy.FORCE;
> > + if (!current) {
> > + if (entry.isCached()) {
> > + entry.markForDelete();
> > + entry.store();
> > + localFile = CacheUtil.makeNewCacheFile(resource.location,
> > resource.downloadVersion);
> > + CacheEntry newEntry = new CacheEntry(resource.location,
> > resource.requestVersion);
> > + newEntry.lock();
> > + entry.unlock();
> > + entry = newEntry;
> > + }
> > + }
> 
> Have you tested this while offline? I am afraid that this might break
> running jnlp applications offline.

If by testing this offline you mean run it using a jnlp file, let it cache, then run jnlp file while disconnected from the internet and still be able to launch it? Then yes, however it fails to launch the cached item. This actually happens with HEAD without my patch too. (Running jnlp file while connected works)

> 
> Everything else looks fine to me.
> 

I've made some minor changes, notably adding an entry for compressed files, since when files are compressed, comparison with extracted file's size against the recorded size will definitely be different (so I avoid this check, have let isCurrent handle the rest by checking last-modified).

Thanks for reviewing my patches!

Cheers,
  Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch999.patch
Type: text/x-patch
Size: 32404 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110416/bb0d9ca2/patch999.patch