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@web.de To: mkgmap-dev@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@web.de To: mkgmap-dev@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@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
_______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
_______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
_______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev