RFC - Reduce loading of PropertiesFile

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

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

Best regards, J.