migrate elemstyle.xml to mapcss

[bastiK]

Now that we have configurable colors in mapcss (thanks to Simon, [6774]) migration of JOSM's default map style to the mapcss format should be possible.

Features, we could use:

• repeat-image to draw cliffs as they appear in Mapnik (e.g. ​here)
• housenumber rendering (#9357)
• zoom levels
• ...

In addition, most user created styles are in mapcss at the moment, so this would make it easier to integrate these styles into the core style.

[Don-vip]

+1, it's a good idea to have a unique map style system :)

Was the configurable color stuff the only showstopper ?

[bastiK]

mapcss version of elemstyles.xml r6988

[bastiK]

I've attached the first version of the converted style. It is not completely identical to the current xml style, especially when it comes to conflicting tags like amentiy=restaurant & tourism=hotel. Please report any differences that are not acceptable and should be fixed!

[bastiK]

The performance is worse. This is expected, as the XML format is simple and very optimized. The performance difference is in the part where the style for each object is generated (and cached). The part where the generated style is painted to screen is unchanged. This means that for detailed mapping in one particular area, there is no difference, but the initial loading of large areas takes longer.

Typically, it looks like this:

# initial load, phase 1 takes long:
phase 1 (calculate styles): 312 ms; phase 2 (draw): 123 ms; total: 435 ms
# doing some stuff in same bounding box, phase 1 negligible, cache is working 
phase 1 (calculate styles): 3 ms; phase 2 (draw): 79 ms; total: 82 ms
phase 1 (calculate styles): 2 ms; phase 2 (draw): 74 ms; total: 76 ms
phase 1 (calculate styles): 2 ms; phase 2 (draw): 71 ms; total: 73 ms
phase 1 (calculate styles): 2 ms; phase 2 (draw): 41 ms; total: 43 ms
phase 1 (calculate styles): 2 ms; phase 2 (draw): 38 ms; total: 40 ms
phase 1 (calculate styles): 3 ms; phase 2 (draw): 50 ms; total: 53 ms
phase 1 (calculate styles): 1 ms; phase 2 (draw): 34 ms; total: 35 ms
phase 1 (calculate styles): 2 ms; phase 2 (draw): 34 ms; total: 36 ms
phase 1 (calculate styles): 2 ms; phase 2 (draw): 30 ms; total: 32 ms
phase 1 (calculate styles): 5 ms; phase 2 (draw): 33 ms; total: 38 ms
# moving map, some new styles need to be generated
phase 1 (calculate styles): 18 ms; phase 2 (draw): 32 ms; total: 50 ms
phase 1 (calculate styles): 25 ms; phase 2 (draw): 34 ms; total: 59 ms
phase 1 (calculate styles): 23 ms; phase 2 (draw): 40 ms; total: 63 ms
phase 1 (calculate styles): 5 ms; phase 2 (draw): 42 ms; total: 47 ms
phase 1 (calculate styles): 2 ms; phase 2 (draw): 42 ms; total: 44 ms
phase 1 (calculate styles): 1 ms; phase 2 (draw): 39 ms; total: 40 ms

In my benchmarks, the time for phase 1 has increased by factor 5-10 with the MapCSS style compared to XML. This is quite a lot, so it would be nice, if the style generation for MapCSS could be optimized.

[stoecker]

As long as we have major difference in speed, we should not drop the XML style.

OTOH MapCSS may nevertheless be the main focus for future improvements and updates. Situations was similar when switching from wireframe to XML format.

[stoecker]

In any case the MapCSS variant should be in JOSM core even if not yet as default.

[bastiK]

In 7041/josm:

see #9691 - include mapcss version of the default style as an option in the map style dialog
not default yet

[bastiK]

In 7042/josm:

see #9691 - update to latest version of xml style

[bastiK]

In 7043/josm:

see #9691 - added command line option to enable debug level trace
added map paint performance output for trace level debug

[bastiK]

In 7054/josm:

mapcss: major performance improvement for style creation, "rendering phase 1" (see #9691)

[bastiK]

In 7055/josm:

mapcss: optimisation by adding an extra class for the most common type of condition (see #9691)

[bastiK]

In [7056]:

mapcss: optimization by converting very hot double loop into single loop (gain ~ 20%)

[bastiK]

In [7057]:

reverting [7056] because it won't work for execution in multiple threads (which will be more useful)

[bastiK]

In 7059/josm:

mapcss: use multiple threads for style creation (see #9691)

significant speed-up on multi core processors
(factor ~3.5 on my 4 core Intel Core i7)

This makes a lot of code run parallel which wasn't written with
multithreading in mind, so keep fingers crossed. :)

[Don-vip]

Don't know if can be of any help, but Java 7 has introduced a new multithreading framework: ​http://docs.oracle.com/javase/tutorial/essential/concurrency/forkjoin.html

[bastiK]

In 7064/josm:

see #9691 - add back reverted optimization [7056] (modified so it is thread safe)

[bastiK]

Replying to Don-vip:

Don't know if can be of any help, but Java 7 has introduced a new multithreading framework: ​http://docs.oracle.com/javase/tutorial/essential/concurrency/forkjoin.html

Sound quite nice. I could probably make use of ​parallel sorting, but it was introduced in Java 8. Well, always one step behind ... :)

[Don-vip]

Replying to bastiK:

but it was introduced in Java 8. Well, always one step behind ... :)

Lot of good stuff in Java 8 :) But IcedTea8 is not even released yet :D You can mark your code with TODO use XXX when switching to Java 8, we'll see these comments around 2016 :) I have started to do so in r7060.

[Don-vip]

In 7066/josm:

see #9691 - fix MapCSSParserTest broken by r7064. MapCSSTagCheckerTest is still broken

[Don-vip]

Are you sure the replacement of multiple Selector by a single one does not kill MapCSSTagChecker for good? The test fails and I'm not sure why.

[bastiK]

Don't worry, should be possible to fix ... but I cannot find the problem either, at the moment.

[bastiK]

In 7069/josm:

see #9691 - fix broken MapCSSTagChecker unit test

[Klumbumbus]

When the migration is done, you could maybe think about replace the default maxspeed icon on nodes (which shows always 60 for every maxspeed value) by a dynamic display of the real maxspeed value. There was already a discussion some years ago in #5859. I think it is better to show the real value instead of always 60. The code which could be integrated in the elemstyle.mapcss could look like this:

node[maxspeed<100]::maxbg
{
	symbol-shape: circle;
	symbol-size: 18;
	symbol-fill-color: white;
}
node[maxspeed<100]::maxfg
{
	symbol-shape: circle;
	symbol-size: 16;
	symbol-stroke-color: crimson;
	symbol-stroke-width: 2;
	text: "maxspeed";
	font-size: 9;
	text-color: black;
	text-anchor-horizontal: center;
	text-anchor-vertical: center;
	text-offset-x: 0;
	text-offset-y: 0;
}
node[maxspeed>=100]::maxbg
{
	symbol-shape: circle;
	symbol-size: 20;
	symbol-fill-color: white;
}
node[maxspeed>=100]::maxfg
{
	symbol-shape: circle;
	symbol-size: 18;
	symbol-stroke-color: crimson;
	symbol-stroke-width: 2;
	text: "maxspeed";
	font-size: 9;
	text-color: black;
	text-anchor-horizontal: center;
	text-anchor-vertical: center;
	text-offset-x: 0;
	text-offset-y: 0;
}

[bastiK]

Good idea, this would be similar to house number rendering.

[Don-vip]

Replying to bastiK:

In 7069/josm:

see #9691 - fix broken MapCSSTagChecker unit test

you forgot to commit the test, it doesn't compile anymore ;)

[bastiK]

In 7073/josm:

see #9691 - really fix broken MapCSSTagChecker unit test

[bastiK]

In 7080/josm:

see #9691 - on single core machine do nothing fancy and stay in EDT - this saves some array copying; provide initial capacities for ArrayLists

[Don-vip]

Can one of you see why this non-regression test fails?
​http://donvip.fr/jenkins/job/JOSM/949/testReport/org.openstreetmap.josm.gui.mappaint.mapcss/MapCSSParserTest/testTicket8071/

[Don-vip]

Also, it would be nice to have a big data file for this performance test:
​http://donvip.fr/jenkins/job/JOSM/949/testReport/junit.framework/TestSuite/org_openstreetmap_josm_gui_mappaint_mapcss_MapCSSPerformanceTest/

[bastiK]

In 7093/josm:

replace current mappaint performance test file by a larger one (but smaller file size due to zipping) (see #9691)

[bastiK]

In 7099/josm:

see #9691 - mapcss: fix concurrency problem with Expressions (cannot save stuff to field of static singlton FUNCTIONS_INSTANCE)

[bastiK]

In 7107/josm:

mappaint: hold read lock on Dataset during painting, so modifications do not lead to inconsistent state (fixes #8671, see #9691)

[bastiK]

I did a few tests:

In the plot you can see the rendering time before and after the optimizations. Starting from r7053, there is only one data point for xml and mapcss because multi-threading wasn't supported at that point. For single thread execution, the xml runtime is unchanged at 740 ms (yellow & blue), however the time for mapcss has dropped from 8915 to 2682 ms (green & red).

Using all threads on the test machine (Intel Core i7), the performance of mapcss is on par with the best xml-style runtime.

--

It is funny how the xml performance gets worse with increasing number of threads. I don't really have an explanation for this at the moment.

To be precise, the graph shows the first phase of rendering (style creation), the second phase always takes about 1100 ms regardless of the optimizations.

[stoecker]

It is funny how the xml performance gets worse with increasing number of threads. I don't really have an explanation for this at the moment.

Multiple threads always have a disadvantage of increased communication costs (locks, IPC, ...). When there are no benefits of multiple threads, then these will have negative effects.

[bastiK]

In 7138/josm:

see #9691 - add tag based index (speed-up of factor 3 in single thread mode)

[bastiK]

Performance for the default style:

• xml single thread: 667 ms
• mapcss single thread: 762 ms
• mapcss multi-thread (4 core): 442 ms

We cannot expect to beat the xml style in single thread execution, as it is relatively simple and therefore should be as fast as you can get. It's pretty close, though. :)

[Don-vip]

Really nice :) Speaking of performance, do we still need ​this test?

If nobody uses it, we should remove it. I'd like to have 100% unit test success.

[bastiK]

In 7139/josm:

see #9691 - switch over to mapcss version of the JOSM map style as default style

[bastiK]

In 7141/josm:

see #9691 - fix unit test

[bastiK]

Replying to Don-vip:

Really nice :) Speaking of performance, do we still need ​this test?

If nobody uses it, we should remove it. I'd like to have 100% unit test success.

I'm not using it, but it was easy to fix.

[Don-vip]

Thanks. I was not sure about a relevant test file and the potential redundancy with StyledMapRendererTest. Still needs a small update to run in headless mode, I look into it.

[Don-vip]

In 7142/josm:

see #9691 - make unit test run in headless mode for continuous integration

[Don-vip]

See #10043 for next improvements that will follow.

[Don-vip]

@bastiK: Did you see that? It looks like rendering is 3 times faster with ​Java 8 (6 min) than ​Java 7 (21 min). I have no idea why. I expect Oracle to make performance improvements at each Java version, but not so much!

[bastiK]

Really astonishing, thanks for pointing that out!