RFC - Reduce loading of PropertiesFile

[Jiri Vanek]

Hi!

diffs as attachments and changelogs as plaintext please :)

Idea looks excelent, but several issues inline:

On 04/10/2012 09:54 PM, Thomas Meyer wrote:
> diff -r 60ef5191add3 netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java
> --- a/netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java	Tue Apr 10 19:10:43 2012 +0200
> +++ b/netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java	Tue Apr 10 21:49:08 2012 +0200
> @@ -108,11 +108,11 @@
>        * Update map for keeping track of recently used items.
>        */
>       public synchronized void load() {
> -        cacheOrder.load();
> +        boolean loaded = cacheOrder.load();
>           /*
>            * clean up possibly corrupted entries
>            */
> -        if (checkData()) {
> +        if (loaded == true&&  checkData()) {
+        if (loaded && checkData()) {
please ;)
(unless it somehow destroy difference between && and & O:) )

>               if (JNLPRuntime.isDebug()) {
>                   new LruCacheException().printStackTrace();
>               }
> @@ -125,7 +125,7 @@
>       /**
>        * check content of cacheOrder and remove invalid/corrupt entries
>        *
> -     * @return true, if cache was coruupted and affected entry removed
> +     * @return true, if cache was corrupted and affected entry removed
>        */
>       private boolean checkData () {
>           boolean modified = false;
> diff -r 60ef5191add3 netx/net/sourceforge/jnlp/util/PropertiesFile.java
> --- a/netx/net/sourceforge/jnlp/util/PropertiesFile.java	Tue Apr 10 19:10:43 2012 +0200
> +++ b/netx/net/sourceforge/jnlp/util/PropertiesFile.java	Tue Apr 10 21:49:08 2012 +0200
> @@ -35,6 +35,9 @@
>
>       /** the header string */
>       String header = "netx file";
> +
> +    /** time of last modification */
> +    long lastStore;
>
>       /** lazy loaded on getProperty */
>       boolean loaded = false;
> @@ -104,24 +107,32 @@
>        * Ensures that the file backing these properties has been
>        * loaded; call this method before calling any method defined by
>        * a superclass.
> +     *
> +     * @return true, if file was (re-)loaded
> +     *         false, if file was still current
>        */
> -    public void load() {
> +    public boolean load() {
>           loaded = true;
Looks like unused?
>
> -        InputStream s = null;
> -        try {
this
> -            if (!file.exists())
> -                return;
should remains here, returning false, as you are testing file.lastModified() later

> +        if(lastStore == 0 || lastStore>  0&&  file.lastModified() != lastStore) {
I believe this lastStore>  0 is redudat:
if(lastStore == 0 || file.lastModified() != lastStore)
will do the same thing?

> +            InputStream s = null;
> +            try {
this
> +                if (!file.exists())
> +                    return false;
will be redundant then (^^^)
> +
> +                try {
> +                    s = new FileInputStream(file);
When you are touching this, I believe the encoding of this file should be during storing and loading 
namely specified as utf-8 (icedtea-web have issues with encodings... so its better to start sooner 
then later)
> +                    load(s);
       *1 (see below ;)
> +                } finally {
> +                    if (s != null) s.close();
> +                }
> +            } catch (IOException ex) {
> +                ex.printStackTrace();
return false here?
> +            }
> +            return true;
no return here here
> +        }
>
> -            try {
> -                s = new FileInputStream(file);
> -                load(s);
> -            } finally {
> -                if (s != null) s.close();
> -            }
> -        } catch (IOException ex) {
> -            ex.printStackTrace();
> -        }
> +        return false;
shouldn't  there be return true/nothing (returns already solved) - considering my hints are valid?
>       }
>
>       /**
> @@ -137,6 +148,7 @@
>                   file.getParentFile().mkdirs();
>                   s = new FileOutputStream(file);
>                   store(s, header);
> +                lastStore = file.lastModified();
This is shouldn't be there. I would like to move lastStore = file.lastModified(); to load function. 
or  to "*1" location (because reason of this timestamp is also external modification of recently_used)
>               } finally {
>                   if (s != null) s.close();
>               }
>
>
Little bit of-the-basin think - are you sure that store() as it is written changes lastModified() 
linux stamp correctly? (I had some bad experiences with this, but have never examined it separately)

Also I would like to suggest at least two tests with this patch
1) generate eg 1mb recently_used file and measure set some deadline how fast javaws  have to start. 
   The value should be failing without this patch, and  passing with this one applied. Also some 
real number how faster this is will be much appreciate!

2) second one will be little bit more difficult. It would be nice to test, that when recently_used 
is modified during javaws run, then it reloads it correctly. And so verify that this patch will not 
cause some regression.


Maybe unittest will be easier to prepare instead of 2)
But I'm afraid those (or similar ) tests will be really necessary.


What do you think?



btw - there is task[1] which have long-term goal to rewrite cache subsystem. Do you want to be 
volunteer?

Thanx for the patch!
Best regards
    J.

[1] http://icedtea.classpath.org/wiki/IcedTea-Web - Caching subsystem replacement assigned to guru 
Omair (omajid at redhat.com)