RFC - Reduce loading of PropertiesFile
Jiri Vanek
jvanek at redhat.com
Mon Apr 16 05:53:24 PDT 2012
On 04/15/2012 10:03 PM, Thomas Meyer wrote:
> 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.
>
argh. I wrote it really wrong :)
My "correction" was directed to loaded == true not to &&.
So what I would like to see is (loaded && checkData()) if nothing serious against.
Sorry for miss-formulating.
>>
>>> 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().
Still looks strange but ok for me for now.
>
>>>
>>> - 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?
fair enough. In your new patch you have fileexists before this statement anyway.
>
>>
>>> + 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.
>
Sorry! My overlook! I'm quite paranoid about new FileReader() which is using platform dependent default encoding, which I consider as one of the fundamental errors in java. (and it is hurting icedtea-web imho when dealing with jnlps in different encoding then utf-8, which windows users are still using )
And I have miss-read FileInputStream. :-/
> 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
omg.. But never mind.. Definitely behind scope of this patch:)
> 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.
Thank you very much in advice.
>
>> What do you think?
>
> sounds good to me.
>
>>
Patch looks ok now (except several nitpickcs inline) but I would like to wait with pushing until tests are done.
>> 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...
And so am I :) But I wanted to make my shot O:)
>
> with kind regards
> thomas
>
>
> 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
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
@@ -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,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
> + if (!file.exists()) {
> + return false;
> + }
> +
loaded = true;
- InputStream s = null;
- try {
- if (!file.exists())
- return;
+ long currentStore = file.lastModified();
+ if(lastStore == 0 || currentStore != lastStore) {
+ InputStream s = null;
+ try {
- try {
- s = new FileInputStream(file);
- load(s);
- } finally {
- if (s != null) s.close();
+ try {
+ s = new FileInputStream(file);
+ load(s);
+ } finally {
+ if (s != null) {
+ s.close();
+ lastStore=currentStore;
+ return true;
+ }
+ }
+ } catch (IOException ex) {
+ ex.printStackTrace();
}
- } catch (IOException ex) {
- ex.printStackTrace();
}
+
+ return false;
}
/**
@@ -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)
} finally {
if (s != null) s.close();
}
Best regards, thanx for patch and looking forward to tests!
J.
More information about the distro-pkg-dev
mailing list