RFC - Reduce loading of PropertiesFile

[Thomas Meyer]

Am Donnerstag, den 12.04.2012, 15:11 +0200 schrieb Jiri Vanek:
> Hi!

Hey!

> diffs as attachments and changelogs as plaintext please :)

oops! Yes, sorry. Here you go:

2012-04-15  Thomas Meyer <thomas at m3y3r.de>

	* netx/net/sourceforge/jnlp/util/PropertiesFile.java
	Reduce no. of disk accesses in (load) when the recently_used file was not
	changed since the last (store).
	* netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java: (load)
	Avoid call of (checkData), when file was not changed.

New patch(es) against tip as attachment.

> 
> 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:) )

&& is the right thing to do here! we want to avoid the call of
checkData(), when the file was not reloaded.

> 
> >               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?

No. This is an already present optimisation to lazy load the file on the
first call of getProperty().

> >
> > -        InputStream s = null;
> > -        try {
> this
> > -            if (!file.exists())
> > -                return;
> should remains here, returning false, as you are testing file.lastModified() later
okay.

> 
> > +        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?

lastModified() tells:
"returns [...] 0L if the file does not exist or if an I/O error occurs"

lastStore will be zero as it is initialised at object creation time.

so I wanted to make sure to at least try to load the file once.
does that makes sense?

> 
> > +            InputStream s = null;
> > +            try {
> this
> > +                if (!file.exists())
> > +                    return false;
> will be redundant then (^^^)

okay.

> > +
> > +                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)

FileInputStream is a byte stream, so no character set is interpreted
here. also the class PropertiesFile is an subclass of Properties.

Properties.load() tells:

"Reads a property list (key and element pairs) from the input byte
stream. The input stream is in a simple line-oriented format as
specified in load(Reader) and is assumed to use the ISO 8859-1 character
encoding; that is each byte is one Latin1 character. Characters not in
Latin1, and certain special characters, are represented in keys and
elements using Unicode escapes."

so I think we should not use a different character set here.

> > +                    load(s);
>        *1 (see below ;)
> > +                } finally {
> > +                    if (s != null) s.close();
> > +                }
> > +            } catch (IOException ex) {
> > +                ex.printStackTrace();
> return false here?

yes, of course. fixed.

> > +            }
> > +            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. 

fixed.

> 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)

no. a fsync() was missing here. fixed.

> 
> 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.

I'll write some test cases and will get some figures.

> What do you think?

sounds good to me.

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

Sorry, but I can only do some small contributions, as this is just a
hobby for me. This sounds more like a full-time job...

with kind regards
thomas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: reduce-load.patch
Type: text/x-patch
Size: 6642 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120415/0e72f3a3/reduce-load.patch