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

[Andrew Su]

Hi,

I have applied your patch and did some adjustments such as using the hash of the url as file name instead of having a really long directory.

Questions? Comments?

Cheers,
  Andrew

----- Original Message -----
> Hi Andrew.
> 
> I've been looking over this. Rather than saying what I
> don't like, I committed your patch to a local repository
> and changed whatever I didn't like. It's much easier to
> express what changes I would like to see in Java than
> in English ;-). I've attached the patch with my changes,
> so in this e-mail I'll just give reasons for those changes.
> 
> A lot of it is pretty minor things (like turning
> "if (e) return false;" into "if (e)\n return false").
> Feel free to ignore those stylistic kind of changes.
> 
> The other changes are:
> Adding a "," between the time and the folder identifier
> in the RecentlyUsed file.
> 
> CacheLRUPolicy: enums are better for singletons. Also, if
> we have a singleton it's not so good to have many static
> fields because if ever we needed more than one instance
> of that class, the synchronized access to the static data
> structures would break.
> 
> CacheLRUPolicy.updateEntry: the changes here save us a
> "substring" call, and it's slightly easier to read.
> 
> getSortedKeys is now called getLRUSortedEntries. This was
> the biggest change and the reasons for it were:
> 1) We already have enough map iterations of the form:
> for key in keyset: value = map.getvalue(key)
> We shouldn't add to these. Iterating over (key, value)
> entries is better.
> 2) It's better to return the entries in the order that
> we want to use them, rather than iterating backwards.
> 3) We want to sort by the last updated time, so it's a
> good idea to turn the strings into longs first. Comparing
> strings might be correct (for now), but it's only
> correct by coincidence, and that should be avoided.
> 
> Regards,
> Denis.
> 
> ----- Original Message -----
> > ----- Original Message -----
> > > On 04/04/2011 05:45 PM, Andrew Su wrote:
> > > >>> I think this patch would be easier to review if you split it
> > > >>> into
> > > >>> two patches - one to fix problems with files being
> > > >>> overwritten,
> > > >>> and another to enforce cache sizes.
> > > > I've removed the section which will ensure the cache limit. Will
> > > > post
> > > > that later.
> > > >
> > >
> > > Thanks!
> > >
> > > > + /**
> > > > + * This will return a File pointing to the location of cache
> > > > item.
> > > > + * @param urlPath Path of cache item within cache directory.
> > > > + * @return File if we have searched before, null otherwise.
> > > > + */
> > > > + private static File getCacheFileIfExist(File urlPath){
> > >
> > >
> > > Instead of nitpicking, I am going to ask you to post some
> > > information
> > > about the files added/modified. I missed this during the original
> > > review
> > > (I suppose that might be a code smell and we need to do some
> > > refactoring), but now I see that this patch introduces and/or
> > > modifies
> > > a
> > > number of files: RecentlyUsed, per cache-entry directory, and
> > > possibly
> > > more metadata. Could you please describe what the overall design
> > > of
> > > this
> > > cache is? What data each file contains? And in what format?
> >
> > File:
> > <UserCacheDir>/RecentlyUsed
> > Desc:
> > Keep track of the most recently accessed file. It can work without
> > this file if we traverse the directory for this information.
> > Format:
> > This file is kept to how a regular properties file works
> > Key=Value
> > The Key consist of time accessed followed by the folder number.
> > Example:
> > 13020173404=<UserCacheDir>/4/http/www.example.come/example.jar
> > In this case the 4 at the end is the cache directory.
> >
> >
> > File:
> > <UserCacheDir>/<FolderNumber>/<Protocol>/<Domain>/<SubDir>/*.info
> > Desc:
> > These files contain information about the file in the directory.
> > Such as file length(size), last-modified header from download, and
> > whether this file should be deleted at shutdown.
> > Format:
> > Each line consist of a Key=Value pair to report the state of the
> > item.
> >
> >
> > The above mentioned files are the only ones that we change.
> >
> > Design:
> > It works similarly to the old cache with the addition of LRU and
> > separate per cache-entry directory.
> > During the initialization of the resource stage
> > (ResourceTracker.initializeResource()) Here we will try and grab
> > the most recently downloaded version of the cache-entry. If this
> > entry does not exist it will prepare a directory to store this
> > item. Now, to prevent another instance of plugin/javaws from
> > overwriting the file we lock the PropertiesFile associated with
> > this entry. If this file does exist, but not up to date then we
> > create a new directory to store the new file. The old entry is
> > marked for deletion at shutdown.
> >
> > Onto the downloading stage. This only happens if we have found
> > the entry to be outdated or does not exist (from above). In here
> > we try to lock on the PropertiesFile, and block if it is either
> > being requested, or already in downloading progress. It is okay
> > to block here because if there is a download for this file in
> > progress, we can't use it until it is completed anyways.
> >
> > Loading the jars... nothing changed here.
> >
> > JVM Shutdown. At this point we will remove all compressed
> > downloads and the folders they extract to since the check for
> > size difference will always be true, it will try to redownload
> > anyways. This can save some space at the end of the day.
> > After removing these entries, it will proceed to removing all
> > folders not being tracked by RecentlyUsed. (To get rid of the
> > currently cached items before patch).
> >
> > >
> > > I would love it if the response consists of patch with a new class
> > > per
> > > file/directory introduced/modified/repurposed and each class
> > > contains
> > > (as comments) the purpose and format :D
> >
> > I've move the policy to it's own class. Where it will handle the
> >
> > I've refactored the managing of the LRU policy into its own class.
> > However modifying info class will not be moved anywhere since that
> > is already handled by CacheEntry. The directories being created
> > didn't seem to fit to be its own class either, can not pass it
> > onto other plugin/javaws instances without having to traverse
> > the directories (I wanted to avoid that).
> >
> > Cheers,
> > Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 20110411_cache_change.patch
Type: text/x-patch
Size: 24028 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110411/c5f3b1c9/20110411_cache_change.patch