RFC - Reduce loading of PropertiesFile
Jiri Vanek
jvanek at redhat.com
Fri May 11 10:23:37 PDT 2012
On 05/11/2012 05:43 PM, Thomas Meyer wrote:
> Am Freitag, den 11.05.2012, 14:49 +0200 schrieb Jiri Vanek:
>> Hi again. Sorry for delay, I was on vacation and then buried under other tasks.
>
> Hi Jiri.
>
> No problem. I hope you had nice vacation!
>
>> I must confess i got lostt little bit. There were a lot of small patches and there were changing
>> sometimes accordingly to review sometimes not, so I probably lost track what was done and what
>> wasn't :(((
>>
>> On 04/30/2012 01:11 PM, Thomas Meyer wrote:
>>> 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:
>>>>>> >
>> snip
>>> All-in-one-patch attached. What do you think about this?
>>
>> yaap! Looks excellent! Two minor nitpicks inline, except them I *believe* all was done.
>
> see my comments below.
>
>>> diff -r 82e908d46d70 netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java
>>> --- a/netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java Tue Apr 24 14:43:34 2012 -0400
>>> +++ b/netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java Mon Apr 30 13:06:12 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()) {
>>
>> there is still horrible loaded == true. just "loaded" please
>
> Opps! This somehow got lost. Fixed in new patch.
>
>>
>>> if (JNLPRuntime.isDebug()) {
>>> new LruCacheException().printStackTrace();
>>> }
>
>>> diff -r 82e908d46d70 netx/net/sourceforge/jnlp/util/PropertiesFile.java
>>> --- a/netx/net/sourceforge/jnlp/util/PropertiesFile.java Tue Apr 24 14:43:34 2012 -0400
>>> +++ b/netx/net/sourceforge/jnlp/util/PropertiesFile.java Mon Apr 30 13:06:12 2012 +0200
>
>>> @@ -104,39 +104,62 @@
>>> * 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() {
>>> - loaded = true;
>>> + public boolean load() {
>>>
>>> - InputStream s = null;
>>> - try {
>>> - if (!file.exists())
>>> - return;
>>> + if (!file.exists()) {
>>> + return false;
>>> + }
>>>
>>> + long currentStore = file.lastModified();
>>> + long currentTime = System.currentTimeMillis();
>>> +
>>> + /* (re)load file, if
>>> + * - it wasn't loaded/stored, yet (lastStore == 0)
>>> + * - current file modification timestamp has changed since last store (currentStore != lastStore) OR
>>> + * - current file modification timestamp has not changed since last store AND current system time equals current file modification timestamp
>>> + * This is necessary because some filesystems seems only to provide accuracy of the timestamp on the level of seconds!
>>> + */
>>> + if(lastStore == 0 || currentStore != lastStore || (currentStore == lastStore&& currentStore / 1000 == currentTime / 1000)) {
>>
>> this looks like magic and is making the things much worse. But it is funny taht even with "just
>> seconds" precisions is making such an improvement.
>
> It's not magic, just some optimization, for a very unlikely case in real
> life, I guess. Above if statement is necessary and not avoidable. I did
> hope to clarify what's going on in the comment.
> I did only catch this very unlikely case because of the unit tests. I
> bet nobody would have notice the missing reload (case 3) in real usage
> for month/years!
>
>>> + InputStream s = null;
>>> 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;
>>> }
>>>
>>> /**
>>> * Saves the properties to the file.
>>> */
>>> public void store() {
>>> - 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();
>>> } finally {
>>> if (s != null) s.close();
>>> }
>
>> Second issue is the usage of
>> JNLPRuntime.getConfiguration().setProperty(DeploymentConfiguration.KEY_USER_CACHE_DIR,
>> System.getProperty("java.io.tmpdir")); ( I know I'm the originally guilty one!). As you done it in
>> BeforClass I started to wonder that t is (and it is) working correctly.
>> However there is no turning back.
>> a) You are not returning the original value
>> b) restoring the original value will not have efect to LruCache.INSTANCE
>
> JNLPRuntime.getConfiguration() is a static method and just returns the
> also static created DeploymentConfiguration object. No load() call is
> done here anywhere. Also no call to save(). So we don't use the
> underlying config file at all here. So no need to restore anything.
>
> Changelog and new patch attached.
>
> with kind regards
> thomas
>
> 2012-05-11 Thomas Meyer<thomas at m3y3r.de>
>
> * tests/netx/unit/net/sourceforge/jnlp/util/PropertiesFileTest.java: Add
> some unit tests for the PropertiesFile class
> * tests/netx/unit/net/sourceforge/jnlp/cache/CacheLRUWrapperTest.java: Add
> some unit tests for the CacheLRUWrapper class
> * netx/net/sourceforge/jnlp/util/PropertiesFile.java: Use last
> modification timestamp of the underlying file to lazy load properties.
> (load): Only reload file, if the file modification timestamp has changed.
> (store): Actually fsync() the file to disk.
> * netx/net/sourceforge/jnlp/services/XPersistenceService.java (create):
> Fix coding style
> * netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java (load): Only check
> data when the recently_used file was reloaded.
>
>
Well go on and push to head!
Thanx for patience and whole this stuff!
J.
More information about the distro-pkg-dev
mailing list