RFC - Reduce loading of PropertiesFile
Jiri Vanek
jvanek at redhat.com
Fri May 11 05:49:13 PDT 2012
Hi again. Sorry for delay, I was on vacation and then buried under other tasks.
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.
>
> with kind regards
> thomas
>
>
>
>
> reduce-load-and-unit-test.patch
>
>
> 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
> 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 82e908d46d70 netx/net/sourceforge/jnlp/services/XPersistenceService.java
> --- a/netx/net/sourceforge/jnlp/services/XPersistenceService.java Tue Apr 24 14:43:34 2012 -0400
> +++ b/netx/net/sourceforge/jnlp/services/XPersistenceService.java Mon Apr 30 13:06:12 2012 +0200
> @@ -127,9 +127,10 @@
> checkLocation(location);
>
> File file = toCacheFile(location);
> - if (!file.exists())
> + if (!file.exists()) {
> throw new FileNotFoundException("Persistence store for "
> + location.toString() + " is not found.");
> + }
> FileUtils.createParentDir(file, "Persistence store for "
> + location.toString());
>
> 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
> @@ -35,9 +35,9 @@
>
> /** the header string */
> String header = "netx file";
> -
> - /** lazy loaded on getProperty */
> - boolean loaded = false;
> +
> + /** time of last modification, lazy loaded on getProperty */
> + long lastStore;
>
> /**
> * Create a properties object backed by the specified file.
> @@ -64,7 +64,7 @@
> * does not exist.
> */
> public String getProperty(String key) {
> - if (!loaded)
> + if (lastStore == 0)
> load();
>
> return super.getProperty(key);
> @@ -75,7 +75,7 @@
> * if the key does not exist.
> */
> public String getProperty(String key, String defaultValue) {
> - if (!loaded)
> + if (lastStore == 0)
> load();
>
> return super.getProperty(key, defaultValue);
> @@ -87,7 +87,7 @@
> * @return the previous value
> */
> public Object setProperty(String key, String value) {
> - if (!loaded)
> + if (lastStore == 0)
> load();
>
> return super.setProperty(key, value);
> @@ -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.
> + 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();
> }
> diff -r 82e908d46d70 tests/netx/unit/net/sourceforge/jnlp/cache/CacheLRUWrapperTest.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/tests/netx/unit/net/sourceforge/jnlp/cache/CacheLRUWrapperTest.java Mon Apr 30 13:06:12 2012 +0200
> @@ -0,0 +1,153 @@
> +/* CacheLRUWrapperTest.java
> + Copyright (C) 2012 Thomas Meyer
> +
> +This file is part of IcedTea.
> +
> +IcedTea is free software; you can redistribute it and/or
> +modify it under the terms of the GNU General Public License as published by
> +the Free Software Foundation, version 2.
> +
> +IcedTea is distributed in the hope that it will be useful,
> +but WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with IcedTea; see the file COPYING. If not, write to
> +the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> +02110-1301 USA.
> +
> +Linking this library statically or dynamically with other modules is
> +making a combined work based on this library. Thus, the terms and
> +conditions of the GNU General Public License cover the whole
> +combination.
> +
> +As a special exception, the copyright holders of this library give you
> +permission to link this library with independent modules to produce an
> +executable, regardless of the license terms of these independent
> +modules, and to copy and distribute the resulting executable under
> +terms of your choice, provided that you also meet, for each linked
> +independent module, the terms and conditions of the license of that
> +module. An independent module is a module which is not derived from
> +or based on this library. If you modify this library, you may extend
> +this exception to your version of the library, but you are not
> +obligated to do so. If you do not wish to do so, delete this
> +exception statement from your version.
> +*/
> +
> +package net.sourceforge.jnlp.cache;
> +
> +import static org.junit.Assert.assertTrue;
> +
> +import java.io.File;
> +
> +import net.sourceforge.jnlp.config.DeploymentConfiguration;
> +import net.sourceforge.jnlp.runtime.JNLPRuntime;
> +
> +import org.junit.BeforeClass;
> +import org.junit.Test;
> +
> +public class CacheLRUWrapperTest {
> +
> + private final CacheLRUWrapper clw = CacheLRUWrapper.getInstance();
> + private final String cacheDir = new File(JNLPRuntime.getConfiguration()
> + .getProperty(DeploymentConfiguration.KEY_USER_CACHE_DIR)).getPath();
> +
> + // does no DeploymentConfiguration exist for this file name?
> + private final String cacheIndexFileName = "recently_used";
> +
> + private final int noEntriesCacheFile = 1000;
> +
> + @BeforeClass
> + static public void setupJNLPRuntimeConfig() {
> + JNLPRuntime.getConfiguration().setProperty(DeploymentConfiguration.KEY_USER_CACHE_DIR, System.getProperty("java.io.tmpdir"));
> + }
> +
> + @Test
> + public void testLoadStoreTiming() throws InterruptedException {
> +
> + int noLoops = 1000;
> +
> + long time[] = new long[noLoops];
> +
> + clw.lock();
> + clearCacheIndexFile();
> +
> + fillCacheIndexFile(noEntriesCacheFile);
> + clw.store();
> +
> + // FIXME: wait a second, because of file modification timestamp only provides accuracy on seconds.
> + Thread.sleep(1000);
> +
> + long sum = 0;
> + for(int i=0; i< noLoops - 1; i++) {
> + time[i]= System.nanoTime();
> + clw.load();
> + time[i+1]= System.nanoTime();
> + if(i==0)
> + continue;
> + sum = sum + time[i] - time[i-1];
> + }
> +
> + double avg = sum / time.length;
> + System.out.println("Average = " + avg + "ns");
> +
> + // wait more than 100 microseconds for noLoops = 1000 and noEntries=1000 is bad
> + assertTrue("load() must not take longer than 100 µs, but took in avg " + avg/1000 + "µs", avg< 100 * 1000);
> +
> + clw.unlock();
> + }
> +
> + private void fillCacheIndexFile(int noEntries) {
> +
> + // fill cache index file
> + for(int i = 0; i< noEntries; i++) {
> + String path = cacheDir + File.separatorChar + i + File.separatorChar + "test" + i + ".jar";
> + String key = clw.generateKey(path);
> + clw.addEntry(key, path);
> + }
> + }
> +
> + @Test
> + public void testModTimestampAfterStore() throws InterruptedException {
> +
> + final File cacheIndexFile = new File(cacheDir + File.separator + cacheIndexFileName);
> +
> + clw.lock();
> +
> + // 1. clear cache entries + store
> + long lmBefore = cacheIndexFile.lastModified();
> + clearCacheIndexFile();
> + long lmAfter = cacheIndexFile.lastModified();
> + assertTrue("modification timestamp hasn't changed! Before = " + lmBefore + " After = " + lmAfter, lmBefore< lmAfter);
> +
> + // FIXME: wait a second, because of file modification timestamp only provides accuracy on seconds.
> + Thread.sleep(1000);
> +
> + // 2. load cache file
> + lmBefore = cacheIndexFile.lastModified();
> + clw.load();
> + lmAfter = cacheIndexFile.lastModified();
> + assertTrue("modification timestamp has changed!", lmBefore == lmAfter);
> +
> + // 3. add some cache entries and store
> + lmBefore = cacheIndexFile.lastModified();
> + fillCacheIndexFile(noEntriesCacheFile);
> + clw.store();
> + lmAfter = cacheIndexFile.lastModified();
> + assertTrue("modification timestamp hasn't changed! Before = " + lmBefore + " After = " + lmAfter, lmBefore< lmAfter);
> +
> + clw.unlock();
> + }
> +
> + private void clearCacheIndexFile() {
> +
> + clw.lock();
> +
> + // clear cache + store file
> + clw.clearLRUSortedEntries();
> + clw.store();
> +
> + clw.unlock();
> + }
> +}
> diff -r 82e908d46d70 tests/netx/unit/net/sourceforge/jnlp/util/PropertiesFileTest.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/tests/netx/unit/net/sourceforge/jnlp/util/PropertiesFileTest.java Mon Apr 30 13:06:12 2012 +0200
> @@ -0,0 +1,151 @@
> +/* PropertiesFileTest.java
> + Copyright (C) 2012 Thomas Meyer
> +
> +This file is part of IcedTea.
> +
> +IcedTea is free software; you can redistribute it and/or
> +modify it under the terms of the GNU General Public License as published by
> +the Free Software Foundation, version 2.
> +
> +IcedTea is distributed in the hope that it will be useful,
> +but WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with IcedTea; see the file COPYING. If not, write to
> +the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> +02110-1301 USA.
> +
> +Linking this library statically or dynamically with other modules is
> +making a combined work based on this library. Thus, the terms and
> +conditions of the GNU General Public License cover the whole
> +combination.
> +
> +As a special exception, the copyright holders of this library give you
> +permission to link this library with independent modules to produce an
> +executable, regardless of the license terms of these independent
> +modules, and to copy and distribute the resulting executable under
> +terms of your choice, provided that you also meet, for each linked
> +independent module, the terms and conditions of the license of that
> +module. An independent module is a module which is not derived from
> +or based on this library. If you modify this library, you may extend
> +this exception to your version of the library, but you are not
> +obligated to do so. If you do not wish to do so, delete this
> +exception statement from your version.
> +*/
> +
> +package net.sourceforge.jnlp.util;
> +
> +import static org.junit.Assert.assertTrue;
> +
> +import java.io.File;
> +import java.io.IOException;
> +import java.nio.channels.FileLock;
> +import java.nio.channels.OverlappingFileLockException;
> +
> +import net.sourceforge.jnlp.config.DeploymentConfiguration;
> +import net.sourceforge.jnlp.runtime.JNLPRuntime;
> +
> +import org.junit.BeforeClass;
> +import org.junit.Test;
> +
> +public class PropertiesFileTest {
> +
> + private int lockCount = 0;
> +
> + /* lock for the file RecentlyUsed */
> + private FileLock fl = null;
> +
> + private final String cacheDir = new File(JNLPRuntime.getConfiguration()
> + .getProperty(DeploymentConfiguration.KEY_USER_CACHE_DIR)).getPath();
> +
> + // does no DeploymentConfiguration exist for this file name?
> + private final String cacheIndexFileName = "recently_used";
> +
> + private final PropertiesFile cacheIndexFile = new PropertiesFile(new File(cacheDir + File.separatorChar + cacheIndexFileName));
> + private final int noEntriesCacheFile = 1000;
> +
> + @BeforeClass
> + static public void setupJNLPRuntimeConfig() {
> + JNLPRuntime.getConfiguration().setProperty(DeploymentConfiguration.KEY_USER_CACHE_DIR, System.getProperty("java.io.tmpdir"));
> + }
> +
> + private void fillCacheIndexFile(int noEntries) {
> +
> + // fill cache index file
> + for(int i = 0; i< noEntries; i++) {
> + String path = cacheDir + File.separatorChar + i + File.separatorChar + "test" + i + ".jar";
> + String key = String.valueOf(System.currentTimeMillis());
> + cacheIndexFile.setProperty(key, path);
> + }
> + }
> +
> + @Test
> + public void testReloadAfterStore() {
> +
> + lock();
> +
> + boolean reloaded = false;
> +
> + // 1. clear cache entries + store
> + clearCacheIndexFile();
> +
> + // 2. load cache file
> + reloaded = cacheIndexFile.load();
> + assertTrue("File was not reloaded!", reloaded);
> +
> + // 3. add some cache entries and store
> + fillCacheIndexFile(noEntriesCacheFile);
> + cacheIndexFile.store();
> + reloaded = cacheIndexFile.load();
> + assertTrue("File was not reloaded!", reloaded);
> +
> + unlock();
> + }
> +
> + private void clearCacheIndexFile() {
> +
> + lock();
> +
> + // clear cache + store file
> + cacheIndexFile.clear();
> + cacheIndexFile.store();
> +
> + unlock();
> + }
> +
> +
> + // add locking, because maybe some JNLP runtime user is running. copy/paste from CacheLRUWrapper
> +
> + /**
> + * Lock the file to have exclusive access.
> + */
> + private void lock() {
> + try {
> + fl = FileUtils.getFileLock(cacheIndexFile.getStoreFile().getPath(), false, true);
> + } catch (OverlappingFileLockException e) { // if overlap we just increase the count.
> + } catch (Exception e) { // We didn't get a lock..
> + e.printStackTrace();
> + }
> + if (fl != null) lockCount++;
> + }
> +
> + /**
> + * Unlock the file.
> + */
> + private void unlock() {
> + if (fl != null) {
> + lockCount--;
> + try {
> + if (lockCount == 0) {
> + fl.release();
> + fl.channel().close();
> + fl = null;
> + }
> + } catch (IOException e) {
> + e.printStackTrace();
> + }
> + }
> + }
> +}
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
If I'm wrong with b, can you please restore original value?
If I'm correct, do you have any idae of impact or of solution/improvement?
best regards from CZ!
J/
More information about the distro-pkg-dev
mailing list