RFC - Reduce loading of PropertiesFile

[Thomas Meyer]

Am Montag, den 16.04.2012, 14:53 +0200 schrieb Jiri Vanek:

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

Hi,

Attached are two patches that implement a unit test of
CacheLRUWrapper.load()

Some numbers of my previous optimisation patches:

No. of entries in the recently_used file = 1000
No. of calls to CacheLRUWrapper.load() = 1000

Before: Average = 3802485.0 ns
After:  Average = 44887.0 ns

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

Still on my todo list :-)

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


>  > diff -r 17f9e4e1ac6d netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java
> --- a/netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java	Wed Apr 11 10:19:17 2012 +0200
> +++ b/netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java	Mon Apr 16 14:08:18 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()) {
> 
>  > As already told above, Unless there is some particular reason, please avoid boolean comparsion with true/false

this is actually fixed in this patch. the attached patch file consists
actually of two diffs that needs to be applied to the icedtea-web repo
in order. mercurial doesn't let me rewrite the commit history, so I just
exported all revisions in one file. that's actually a bit confusing,
when you only look at the patch file.


> 
>               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 17f9e4e1ac6d netx/net/sourceforge/jnlp/util/PropertiesFile.java
> --- a/netx/net/sourceforge/jnlp/util/PropertiesFile.java	Wed Apr 11 10:19:17 2012 +0200
> +++ b/netx/net/sourceforge/jnlp/util/PropertiesFile.java	Mon Apr 16 14:08:18 2012 +0200
> @@ -104,24 +107,38 @@
>        * 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() {
> +
> +        if (!file.exists())
> +            return false;
> 
>  > this is really nitpick and I'm sorry for it, but current code guidelines dictate brackets after if

okay, will fix this, too.

> @@ -131,12 +148,16 @@
>           if (!loaded)
>               return; // nothing could have changed so save unnecessary load/save
> 
> -        OutputStream s = null;
> +        FileOutputStream s = null;
>           try {
>               try {
>                   file.getParentFile().mkdirs();
>                   s = new FileOutputStream(file);
>                   store(s, header);
> +
> +                // fsync()
> +                s.getChannel().force(true);
> +                lastStore = file.lastModified();
>  > Very well :) I have never seen this before, so I hope you know what you are doing:) (but looks correct from javadoc)

I'm not sure if a flush() is needed before the fsync() aka.
s.getChannel().force(true);
Internet suggest a flush() is needed. I may add this. Suggestions?

thomas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch1.diff
Type: text/x-patch
Size: 1261 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120419/cca3637c/patch1.diff 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch2.diff
Type: text/x-patch
Size: 4186 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120419/cca3637c/patch2.diff