Opened 17 months ago
Last modified 5 months ago
#23008 reopened enhancement
[WIP patch] Improve PowerLines test performance
Reported by: | gaben | Owned by: | team |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | Core | Version: | |
Keywords: | power performance | Cc: |
Description
I have to check why (what and why I missed previously), but this single addition dramatically improves the test execution time. The test was extended in r18553.
-
src/org/openstreetmap/josm/data/validation/tests/PowerLines.java
628 628 * @return {@code true} if primitive has a tag that marks it as a water area or boundary of a water area 629 629 */ 630 630 private static boolean concernsWaterArea(OsmPrimitive p) { 631 return p.hasTag("water", "river", "lake") || p.hasKey("waterway") || p.hasTag("natural", "coastline");631 return (p.hasTag("water", "river", "lake") || p.hasKey("waterway") || p.hasTag("natural", "coastline")) && p.concernsArea(); 632 632 } 633 633 634 634 /**
(Currently migrating to a new dev environment so publishing even the WIP patches.)
Attachments (3)
Change History (19)
comment:1 by , 14 months ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:2 by , 14 months ago
Milestone: | → 23.09 |
---|
comment:3 by , 13 months ago
I wonder in what case ways with natural=coastline will be used after this patch.
comment:6 by , 13 months ago
I did test it in my standard test area (Mesa County, CO
) which has no coastlines...
Anyway, this is why I like tests. It keeps me from borking stuff accidentally.
I'll double check, but when I was looking at this, I did check what concernsArea
does. Which is comparing tags against a set of known area tags, and (if it is a relation) check to see if the relation is a multipolygon.
EDIT: To clarify, I did think about just using the relation multipolygon check instead of concernsArea, but I though concernsArea was more readable.
comment:7 by , 13 months ago
You'll probably never see a complete area built with natural=coastline. OTOH we may exclude small waterways like ditch, drain, stream as these are probably never a reason for a longer distance between two power towers?
comment:8 by , 13 months ago
Oddly enough, the concernsArea
call seems to have no effect on the tests I wrote. Can someone check them for sanity?
by , 13 months ago
Attachment: | 23008-sample.osm added |
---|
test data that should show no warnings from powerline test
comment:9 by , 13 months ago
Try with the attached sample. When you remove the coastline way the powerline test complains. Tested with r18822
comment:11 by , 13 months ago
Milestone: | 23.09 |
---|---|
Resolution: | fixed |
Status: | closed → reopened |
comment:12 by , 13 months ago
I have to test, but the code with concernsArea()
is still better in my opinion. Can someone estimate how many cases could be where the power line crosses coastlines like the example data?
Probably there aren't much, and until then the performance improvement is a welcomed change.
by , 13 months ago
Attachment: | 23008-sample2.osm added |
---|
comment:13 by , 13 months ago
I'd guess that most of the speed improvement could also be done with primitive instanceof Relation && ((Relation) primitive).isMultipolygon()
.
comment:14 by , 13 months ago
Reg. coastline: I gave an example here https://josm.openstreetmap.de/ticket/20716#comment:28
I guess it is a rare problem, but coastline ways should not have a big impact on performance. Small waterways are much more likely to appear in data and are also likely to "cross" power lines.
A possible performance improvement might be to perform the crossing ways test only when missing power nodes where found. The current implementation collects and indexes all waterway objects even if no powerlines exist in the data, right?
comment:15 by , 13 months ago
The current implementation collects and indexes all waterway objects even if no powerlines exist in the data
It should, yes. It would be much more expensive to go back and revisit all ways after finding a powerline.
With that said, we could try adding a new collection (e.g. foundWaterways
) and we run the indexing in endTest
if, and only if, we need it for powerlines.
comment:16 by , 5 months ago
Note to myself: maybe I could utilize QuadBuckets somewhere in this test to gain some performance, possibly replace the existing "line hanging over a water area" part.
In 18838/josm: