Modify

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#9691 closed enhancement (fixed)

migrate elemstyle.xml to mapcss

Reported by: bastiK Owned by: team
Priority: normal Milestone: 14.05
Component: Core mappaint Version:
Keywords: Cc: simon04

Description

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.

Attachments (2)

elemstyles.mapcss (74.2 KB) - added by bastiK 5 years ago.
mapcss version of elemstyles.xml r6988
mapaint-performance.png (24.8 KB) - added by bastiK 5 years ago.

Download all attachments as: .zip

Change History (47)

comment:1 Changed 5 years ago by Don-vip

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

Was the configurable color stuff the only showstopper ?

Changed 5 years ago by bastiK

Attachment: elemstyles.mapcss added

mapcss version of elemstyles.xml r6988

comment:2 Changed 5 years ago by 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!

comment:3 Changed 5 years ago by 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.

comment:4 Changed 5 years ago by 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.

comment:5 Changed 5 years ago by stoecker

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

comment:6 Changed 5 years ago by bastiK

In 7041/josm:

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

comment:7 Changed 5 years ago by bastiK

In 7042/josm:

see #9691 - update to latest version of xml style

comment:8 Changed 5 years ago by bastiK

In 7043/josm:

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

comment:9 Changed 5 years ago by bastiK

In 7054/josm:

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

comment:10 Changed 5 years ago by bastiK

In 7055/josm:

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

comment:11 Changed 5 years ago by bastiK

In [7056]:

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

comment:12 Changed 5 years ago by bastiK

In [7057]:

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

comment:13 Changed 5 years ago by 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. :)

comment:14 Changed 5 years ago by 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

comment:15 Changed 5 years ago by bastiK

In 7064/josm:

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

comment:16 in reply to:  14 ; Changed 5 years ago by 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 ... :)

comment:17 in reply to:  16 Changed 5 years ago by 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.

comment:18 Changed 5 years ago by Don-vip

In 7066/josm:

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

comment:19 Changed 5 years ago by 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.

comment:20 Changed 5 years ago by bastiK

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

comment:21 Changed 5 years ago by bastiK

In 7069/josm:

see #9691 - fix broken MapCSSTagChecker unit test

comment:22 Changed 5 years ago by 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;
}

comment:23 Changed 5 years ago by bastiK

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

comment:24 in reply to:  21 Changed 5 years ago by 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 ;)

comment:25 Changed 5 years ago by bastiK

In 7073/josm:

see #9691 - really fix broken MapCSSTagChecker unit test

comment:26 Changed 5 years ago by 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

comment:29 Changed 5 years ago by bastiK

In 7093/josm:

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

comment:30 Changed 5 years ago by bastiK

In 7099/josm:

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

comment:31 Changed 5 years ago by bastiK

In 7107/josm:

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

Changed 5 years ago by bastiK

Attachment: mapaint-performance.png added

comment:32 Changed 5 years ago by 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.

comment:33 in reply to:  32 Changed 5 years ago by 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.

comment:34 Changed 5 years ago by bastiK

In 7138/josm:

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

comment:35 Changed 5 years ago by 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. :)

comment:36 Changed 5 years ago by 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.

comment:37 Changed 5 years ago by bastiK

In 7139/josm:

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

comment:38 Changed 5 years ago by bastiK

In 7141/josm:

see #9691 - fix unit test

comment:39 in reply to:  36 Changed 5 years ago by 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.

comment:40 Changed 5 years ago by 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.

comment:41 Changed 5 years ago by Don-vip

In 7142/josm:

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

comment:42 Changed 5 years ago by Don-vip

Milestone: 14.05

comment:43 Changed 5 years ago by Don-vip

Resolution: fixed
Status: newclosed

See #10043 for next improvements that will follow.

comment:44 Changed 5 years ago by 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!

comment:45 Changed 5 years ago by bastiK

Really astonishing, thanks for pointing that out!

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.