[mkgmap-dev] [PATCH V3]mkgmap performance

Sun Jan 1 19:31:05 GMT 2012

Hi Gerd,

>
> WanMil wrote
>>
>> * Can you please add some javadoc to the new methods? I know the old
>> methods often do not have javadocs but that shouldn't be continued...
>>
> I totally agree. The attached correction contains comments. I am a little
> bit in trouble with this because the new methods and the corresponding flags
> in Coord are only meaningfull inside the existing
> removeShortArcsByMergingNodes() method of ElementSaver, but I think the
> performance improvement is worth enough to allow this rather bad coding
> style.

Did you try to use another storage mechanism within the 
removeShortArcs... method? I don't like storing the infos of the short 
arcs in *all* coords. Your patch reduces the memory footprint of the 
Coord object (great!). But I think information should be as local as 
possible.

>
>
>
>> * RuleIndex.getRulesForTag returns a BitSet that can be modified
>> externally. This is no good style. I know that the BitSet is not
>> modified externally so it's not a bug. But at least is should be
>> documented in the javadoc that the returned BitSet *must not* be modified.
>>
> In fact, the method was wrong because it evtl. modified by mistake the
> BitSet in tagVals. This is corrected with the new patch which returns a
> newly created BitSet.

Great!

>
>
>> * I propose to use an ArrayList instead of HashSet in line 157 of
>> RuleIndex
>>
> OK, changed. This part of the code is not performance critical, so I wanted
> to keep the changes small.
>
>
>
>> * RuleIndex line 175: The set.clear(i) method changes the BitSet in
>> tagVals/tagnames directly. The unpatched mkgmap is working on a copy.
>> Can you please check carefully if your version is correct?
>>
> Good catch, my version was definetly wrong. This is also corrected with the
> new patch.
>
> The new patch adds one more small change:
> In removeShortArcsByMergingNodes() Coord.distance() is only called if we
> really have to calculate the length of the arc. If parameter
> remove-short-arcs is used without a value (or with remove-short-arcs=0),
> there is no need to calculate the distance, we just need to know if the two
> points are equal.
>
> I also have one question:
> What is causing mkgmap to produce different results each time when it
> executes? This makes optimization very difficult because one cannot simply
> compare old and new result.
>
> http://gis.638310.n2.nabble.com/file/n7129718/mkgmap_performance_3.patch
> mkgmap_performance_3.patch
>
> Ciao,
> Gerd
>

All in all I see speed improvements using the patch. So after the Coord 
question is cleared I would like to commit that if nobody vetoes and you 
agree.

Thanks!
WanMil