RFC - Reduce loading of PropertiesFile
Thomas Meyer
thomas at m3y3r.de
Fri May 11 08:43:25 PDT 2012
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: reduce-load-and-unit-test.patch
Type: text/x-patch
Size: 16813 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120511/fdb6e3cf/reduce-load-and-unit-test.patch
More information about the distro-pkg-dev
mailing list