[mkgmap-dev] Merging back the performance branch

Thu Mar 8 09:53:55 GMT 2012

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.mkgmap.org.uk/pipermail/mkgmap-dev/attachments/20120308/c79c404e/attachment.html 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: findbugs_v1.patch
Type: application/x-download
Size: 1809 bytes
Desc: not available
Url : http://lists.mkgmap.org.uk/pipermail/mkgmap-dev/attachments/20120308/c79c404e/attachment.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: performance_cleanup_v1.patch
Type: application/x-download
Size: 42116 bytes
Desc: not available
Url : http://lists.mkgmap.org.uk/pipermail/mkgmap-dev/attachments/20120308/c79c404e/attachment-0001.bin 