RFC - Reduce loading of PropertiesFile

[Thomas Meyer]

Am Freitag, den 20.04.2012, 12:25 +0200 schrieb Jiri Vanek:
> On 04/19/2012 09:14 PM, Thomas Meyer wrote:
> > 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?
> >
> 
> Only naive one - flush() fill certainly not make thinks worse. So I'm for.

flush() is not needed and is already done in the Properties.store()
method.

> 
> Nice work till now! Let's wait for last test and looking forward for push!

All-in-one-patch attached. What do you think about this?

with kind regards
thomas


-------------- next part --------------
A non-text attachment was scrubbed...
Name: reduce-load-and-unit-test.patch
Type: text/x-patch
Size: 16821 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120430/5f2fd439/reduce-load-and-unit-test.patch