[mkgmap-dev] Merging back the performance branch

Thu Mar 8 19:57:36 GMT 2012

Hi Gerd,

regards the findbugs fixes:
I have commited the StyleTester change. And I have changed the 
LocationHook synchronization to use a separate static lock object. Using 
the boundaryDirName could have the same problem like using BOUNDS_OPTION.

I am unsure about the other findbugs changes (GmapsuppBuilder and 
TypCompiler).
They look good to me but Steve should know better if they are a good fix 
or if they remove a required side effect?!?

WanMil

> Hi WanMil,
>
> attached are two patches:
> 1) findbugs found several problems in the existing sources.
> findbugs.patch contains my suggested solution for three of them,
> please review
>
> 2) performance_cleanup_v1.patch contains
> - updated javadoc
> - removed dead or commented code
> - private constructor for stand-alone BoundaryPreparer to get
> rid of new parameters like "preparer-out-dir"
> - System.out.println -> log.*
> - moved readArea() to readStreamRawFormat to get closer to the
> trunk version
> - LocationHook: findbugs doesn't like
> synchronized (BOUNDS_OPTION) {...}
> because BOUNDS_OPTION is an interned String. The
> patch changes that to
> synchronized (boundaryDirName){...}
> I am not sure if that is better?
>
> Gerd
>
>
>  > Date: Sun, 4 Mar 2012 17:18:41 +0100
>  > From: wmgcnfg at web.de
>  > To: mkgmap-dev at lists.mkgmap.org.uk
>  > Subject: Re: [mkgmap-dev] Merging back the performance branch
>  >
>  > > Hi WanMil,
>  > >
>  > >
>  > > > Date: Sun, 4 Mar 2012 16:28:12 +0100
>  > > > From: wmgcnfg at web.de
>  > > > To: mkgmap-dev at lists.mkgmap.org.uk
>  > > > Subject: [mkgmap-dev] Merging back the performance branch
>  > > >
>  > > > Hi Gerd,
>  > > >
>  > > > in your opinion what is the current status of the performance branch?
>  > > > There seem to be no more fundamental changes so I would like to
> focus on
>  > > > merging it back to the trunk. Additional improvements can then be
>  > > > applied to the trunk.
>  > >
>  > > well, r2234 doesn't contain all patches that I've posted. Do you
> plan to
>  > > add them?
>  >
>  > Of course, but I want to have a look on the patches before commiting
> them.
>  >
>  > >
>  > > My latest version contains another big change regarding Tags and
>  > > StyledConverter.
>  > > I'd like to change the Rule evaluation in a way that it will allow to
>  > > skip many rules
>  > > that are now partly evaluated.
>  > > Up to now my changes are saving maybe 5% - 10 %, but I hope to see
> more.
>  > > I don't think that this will be done soon.
>  >
>  > We can merge back the performance branch and continue using it for the
>  > style changes.
>  >
>  > >
>  > > I have an idea how splitArea() could be changed to be much faster, but
>  > > no algorithm
>  > > yet. I believe that we don't need any Area.intersect() calculations for
>  > > that, we
>  > > just have to split a few lines.
>  >
>  > That's great.
>  > Maybe you can invest your time better in other points of mkgmap with a
>  > greater improvement. Performance improvements are great but there are
>  > some functional problems unsolved, e.g. the Subdivision creation in
>  > mkgmap has some problems which need to be addressed.
>  >
>  > >
>  > > >
>  > > > The performance improvements are great. Before merging back I
> would like
>  > > > to invite people (and give them the chance) to test it carefully. For
>  > > > this I will upload the complete world bounds so that anyone can
>  > > participate.
>  > >
>  > > good. I can't believe that so many changes do not contain any bugs:-)
>  > >
>  > > >
>  > > > From my point of view the following things must be done before
> merging
>  > > > back:
>  > > > * Remove several System.out printouts. Please use the mkgmap internal
>  > > > logging system.
>  > > > * One note to the logging system: Instead of using
>  > > > log.info("There were "+n+" results")
>  > > > better use
>  > > > log.info("There were", n, "results")
>  > > > This avoids string creation in case the log message is not logged.
>  > >
>  > > okay, I will change that.
>  > >
>  > > > * Please check the code with FindBugs or something similar
>  > >
>  > > okay, I wasn't aware that this tool is for free :-)
>  > >
>  > > > * Is the source code well documented?
>  > >
>  > > hard to say. I tend to comment only those lines that implement
>  > > a tricky solution. Everything else should be self-documenting.
>  > >
>  > > > * Is the licence header added to all classes?
>  > > What would the right one?
>  >
>  > I don't know if there is a 'correct' one but I am using the following:
>  > /*
>  > * Copyright (C) 2006, 2012.
>  > *
>  > * This program is free software; you can redistribute it and/or modify
>  > * it under the terms of the GNU General Public License version 3 or
>  > * version 2 as published by the Free Software Foundation.
>  > *
>  > * This program 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.
>  > */
>  >
>  > >
>  > > > * The BoundaryPreparer uses options like "preparer-out-dir". Are
> these
>  > > > options documented? The name should be more specific (e.g.
>  > > bounds-out-dir).
>  > > okay
>  > >
>  > > Gerd
>  > >
>  > >
>  > > >
>  > > > WanMil
>  > > > _______________________________________________
>  > > > mkgmap-dev mailing list
>  > > > mkgmap-dev at lists.mkgmap.org.uk
>  > > > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
>  > >
>  > >
>  > > _______________________________________________
>  > > mkgmap-dev mailing list
>  > > mkgmap-dev at lists.mkgmap.org.uk
>  > > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
>  >
>  > _______________________________________________
>  > mkgmap-dev mailing list
>  > mkgmap-dev at lists.mkgmap.org.uk
>  > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
>
>
> _______________________________________________
> mkgmap-dev mailing list
> mkgmap-dev at lists.mkgmap.org.uk
> http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev