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

[Denis Lila]

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: myMods.patch
Type: text/x-patch
Size: 11859 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110406/a72a9a5e/myMods.patch