[RFC][Icedtea-web]: Changed how cache works.
Denis Lila
dlila at redhat.com
Thu Apr 14 10:54:01 PDT 2011
Hi.
You forgot to hg add CacheLRUWrapper. Assuming it was just a
rename of the corresponding file in the previous version of
your patch, I added it to your patch, and attached it. The
attached patch should compile.
Here are some things that jump out at me (more comments may come
later on):
In getCacheFileIfExists, can you put the lastPath computation
in it's own function (similar to getIdForCacheFolder). That way
it'll be a bit easier to figure out what's going on.
In removeUntrackedDirectories you do
+ for (File f : temp.listFiles()) {
+ String path = f.getPath();
+ temp = new File(path);
+ if (temp.isDirectory() && !keep.contains(temp.getPath())) {
+ try {
+ FileUtils.recursiveDelete(temp, temp);
Wouldn't it be the same (and simpler) to do:
+ for (File f : temp.listFiles()) {
+ if (f.isDirectory() && !keep.contains(f.getPath())) {
+ try {
+ FileUtils.recursiveDelete(f, f);
Also, the HashSet<String> argument of that function is called "keep".
This is a bit peculiar, since you're deleting everything in it.
Also, can you make that a Set<String> instead of HashSet?
Most importantly:
> 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.
Isn't this broken? I mean, what happens when two different urls
hash to the same code?
To me, it seems like you're just going to create a file when it
is first hashed, and when a different URL has the same hash
getCachFileIfExist will return the first cached file, instead
of creating a new one.
Regards,
Denis.
----- Original Message -----
> Hi,
>
>
> 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: asu_cache_v2.patch
Type: text/x-patch
Size: 30850 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110414/7f421089/asu_cache_v2.patch
More information about the distro-pkg-dev
mailing list