Render unpaved roads different

[sommerluk]

Fixes #110 using the idea from #2640 while trying to avoid re-introducing #3280

[sommerluk]

@rrzefox Could you test this branch on your server giving some feedback about performance on different zoom levels, compared to the current master?

[rrzefox]

I have tried to simply render a 2048x1024 tile with nik4.py on zoomlevels 8 to 12. The server has a full planet DB. The results are not good. Each test was run 3 times, once for warmup, and the following table gives the best of the other 2 results:

Zoomlevel     without_this_PR     with_this_PR
   08              82                 119
   09              30                  43
   10              53                 188
   11              25                  70
   12              15                  35

Under these circumstances, I really do not want to deploy this into the live rendering ;)

benchmark output

unpatched run 2:
URL 0 (https://www.openstreetmap.org/#map=12/49.6100/11.0262):

real 0m20.997s
user 0m5.460s
sys 0m0.528s
URL 1 (https://www.openstreetmap.org/#map=11/49.5096/11.0625):

real 0m34.405s
user 0m8.904s
sys 0m0.560s
URL 2 (https://www.openstreetmap.org/#map=10/49.5626/10.9850):

real 0m54.297s
user 0m19.280s
sys 0m0.940s
URL 3 (https://www.openstreetmap.org/#map=9/49.7049/10.8820):

real 0m29.649s
user 0m16.268s
sys 0m1.108s
URL 4 (https://www.openstreetmap.org/#map=8/49.539/10.305):

real 1m21.594s
user 0m33.612s
sys 0m1.892s
Rendering benchmark took 221 seconds total.

unpatched run 3:
URL 0 (https://www.openstreetmap.org/#map=12/49.6100/11.0262):

real 0m15.126s
user 0m5.436s
sys 0m0.544s
URL 1 (https://www.openstreetmap.org/#map=11/49.5096/11.0625):

real 0m24.917s
user 0m9.248s
sys 0m0.620s
URL 2 (https://www.openstreetmap.org/#map=10/49.5626/10.9850):

real 0m53.036s
user 0m19.380s
sys 0m1.200s
URL 3 (https://www.openstreetmap.org/#map=9/49.7049/10.8820):

real 0m31.206s
user 0m17.304s
sys 0m1.208s
URL 4 (https://www.openstreetmap.org/#map=8/49.539/10.305):

real 1m23.309s
user 0m34.644s
sys 0m2.412s
Rendering benchmark took 208 seconds total.

patched run 2:
URL 0 (https://www.openstreetmap.org/#map=12/49.6100/11.0262):

real 0m35.593s
user 0m9.564s
sys 0m1.212s
URL 1 (https://www.openstreetmap.org/#map=11/49.5096/11.0625):

real 1m11.907s
user 0m17.284s
sys 0m2.156s
URL 2 (https://www.openstreetmap.org/#map=10/49.5626/10.9850):

real 3m11.256s
user 0m40.440s
sys 0m5.844s
URL 3 (https://www.openstreetmap.org/#map=9/49.7049/10.8820):

real 0m43.480s
user 0m19.144s
sys 0m1.876s
URL 4 (https://www.openstreetmap.org/#map=8/49.539/10.305):

real 2m1.607s
user 0m42.376s
sys 0m3.752s
Rendering benchmark took 464 seconds total.

patched run 3:
URL 0 (https://www.openstreetmap.org/#map=12/49.6100/11.0262):

real 0m35.349s
user 0m9.480s
sys 0m1.204s
URL 1 (https://www.openstreetmap.org/#map=11/49.5096/11.0625):

real 1m10.434s
user 0m17.092s
sys 0m2.156s
URL 2 (https://www.openstreetmap.org/#map=10/49.5626/10.9850):

real 3m7.632s
user 0m40.872s
sys 0m5.304s
URL 3 (https://www.openstreetmap.org/#map=9/49.7049/10.8820):

real 0m43.129s
user 0m19.108s
sys 0m1.432s
URL 4 (https://www.openstreetmap.org/#map=8/49.539/10.305):

real 1m59.042s
user 0m41.592s
sys 0m3.652s
Rendering benchmark took 456 seconds total.

[imagico]

Based on a few quick tests without a global database it seems that even without the global bounding box geometries and the comp-op stuff the restructured MSS code is significantly slower than before. The SQL code seems to be fine now OTOH.

It has been some time since i looked over this so forgive me if this is stupid but can't you sort the global bounding box (in several instances, one for each road color) into the SQL defined drawing order and this way get rid of the attachments in the MSS code (and this way keep the MSS structure essentially as it is now - just adding the comp-op stuff)?

[timautin]

@rrzefox Thanks for sharing these numbers. Could you add some information about your hardware please?

[sommerluk]

@rrzefox Thanks a lot for testing this!

even without the global bounding box geometries and the comp-op stuff the restructured MSS code is significantly slower than before

That’s what I suspected earlier (see the last point of my previous comment).

[…] can't you sort the global bounding box […] into the SQL defined drawing order […]?

I fear that I can’t. The reason is the behaviour of comp-op. The following occured when rendering just by SQL order without attachments:

0. We have yet various things like landcovers rendered. Let’s call this canvas A.
1. Render white unpaved roads: Cut a hole in the existing image by using comp-op. Now we have canvas A with some holes.
2. Render the white pattern behind: It will be only visible where we have the holes in canvas A that have been done in the previous step. Now we have still canvas A with some holes. And we have canvas B with the white pattern on the global bounding box. Canvas B is behind canvas A.
3. Render white paved roads. They will be rendered in canvas A.
4. Render yellow unpaved roads: Cut a hole in the existing image by using comp-op. This cuts more holes in canvas A (only!).
5. Render the yellow pattern behind: This will create canvas C, which will be behind canvas B. It will therefore never be visible.

[imagico]

That is unfortunate.

But what you describe to me kind of contradicts the mapnik docs because what you do is what in

https://github.com/mapnik/mapnik/wiki/Compositing

is described as Feature level compositing which should work on a per feature basis and not involve the creation of temporary canvas (which is described below as Style level compositing).

The lack of reliable documentation of how mapnik works really hampers developing a solution to this kind of problem.

[kocio-pl]

Well, maybe you could just ask Mapnik developers then - @talaj, would you have any advise here?

[imagico]

Adding cache-features: on to all layers involved as per mapnik/mapnik#3977 (comment) seems to improve performance but i have not tested this in more detail.

That's a heck of an unexpected default setting.

[kocio-pl]

It looks like upstream (Mapnik) implementation of roads with pattern rendering is at least doable - mapnik/mapnik#3977 (comment). That would be really great...

[kocio-pl]

Even better - it's a real work in progress! See mapnik/mapnik#3980.

[sommerluk]

Rebased.

I’ve implemented the performance tuning that @talaj had proposed.

Adding cache-features: on

That didn’t work. I eventually used cache-features: true instead.

Now, on my local machine (based in single tile rendering once per zoom level), the branch unpaved20 is even slightly faster the current master branch. @rrzefox Could you confirm this?

[imagico]

That didn’t work. I eventually used cache-features: true instead.

Strange - because for me it did work.

It would also make sense by the way to check if other layers would profit from this as well. Roads is the most obvious candidate but it is not the only place attachments are used.

[sommerluk]

check if other layers would profit from this as well

Indeed I was thinking about that, but I wanted to wait for the results of @rrzefox before. In general, I suppose that this option is without any negative consequences on layers that do not use attachments. If so, I could imagine add this in general to all SQL layers, regardless of their current usage. This way, we do not need to worry about. (I’m not sure about non-SQL layers however…)

[kocio-pl]

Please remember to look also at memory consumption - this was a big problem that made this option to be turned off by default, see mapnik/mapnik#657.

Do you think this PR still makes sense when there's a Mapnik feature for rendering line patterns being implemented (mapnik/mapnik#3980)?

[rrzefox]

OK, so I reran the simple benchmarks, and they indeed do look a LOT better now:

Zoomlevel     w/o_this_PR   this_PR_oldrev   this_PR_newrev
   08              82          119                80
   09              30           43                29
   10              53          188                56
   11              25           70                26
   12              15           35                17

In other words, the performance impact is now zero (within margin of error).

As for the memory usage, the maximum memory usage was 1529592 KB for the Z08 tile. The same tile without this PR used 1424076 KB, so it's +7%. That is only the memory used by mapnik, no idea how much the corresponding database process used.

I also tried the 'evil' Z10 tile mentioned in the linked mapnik-bugreport. The memory usage for that one increases from 872468 to 1007784 KB (+16%). Fun fact: with the old revision of this PR this actually used 4056448 KB!

I have now deployed this PR, but no tiles have been rerendered yet and all tiles have been rerendered now. Rerendering did seem to take longer than usual, but right now I cannot supplement that "feeling" with any numbers.

[sommerluk]

Thanks a lot @rrzefox for testing this! It’s really great to see this live on a map!

A possible support in Mapnik for direct polygon pattern rendering on lines might come in the future, but it will take a very long time until being available (Mapnik code is currently just a proposal, Mapnik release, than Cartocss release with the necessary changes considering this new symbolizer, Ubuntu packaging, new Ubuntu long term release and the first following maintance release might be maybe 20.4.1?) A lot of conditions…

I would like therefore love to see this PR in openstreetmap-carto in the meantime.

[kocio-pl]

I see. How hard do you think it will be to switch to the Mapnik support with this code? If this is possible to make smooth migration once it's available, I would have no problem merging it.

Depending on circumstances making Mapnik implementation available on OSMF servers might take few weeks or months probably (it's just my rough estimation). It took almost a year with fixing label placement bug on production, but there were more problems along the way.

[sommerluk]

How hard do you think it will be to switch to the Mapnik support with this code?

Quite easy.

1. Delete the attachments and the global bounding box SQL stuff and so on…
2. Than add the rules for the polygon pattern symbolizer. (Given that the current Mapnik proposal proposes using polygon-pattern symbolizer, we will need each road width twice: once for the full-colour roads at line symbolizer, and once again for the pattern roads with the polygon-pattern symbolizer.)

We would still use the same SVG files for the pattern.

After that, the code structure will be less complex. The first step will reduce the lines of code. The second one will raise them again. It is more verbose, but less complex and more straightforward. All in all, I think will will end up with roughly the same amount of code.

[kocio-pl]

OK, it sounds reasonable for me!

Do you think we should merge it now or wait until v4.15.0 is out? Personally I would not wait, since the code will rather not get any more testing than it already did.

[sommerluk]

I’m fine with merging it now.

[kocio-pl]

@giggls @jbelien (I'm not aware who else is syncing forks with us) - are you OK with merging it now or you need something?

[giggls]

The merge will likely make a lot of work again, as like last time almost all of roads.mss has been changed.
This said, I kept my manual merge from last time in a separate branch to be able to restart from there as an option. So yes, go ahead with merging this patch as it now does not seem to kill performance any more.

[kocio-pl]

I have tested it roughly and it works OK, so I give 👍 for merging.

[rrzefox]

I now have some numbers to support my gut feeling that "rerendering seemed to take longer than usual" mentioned above:
Averaging over all tiles rendered on a particular day, I get the following results for seconds per tile:

    |         OLD  |          OLD |         OLD  |         NEW  |         NEW
  z |  2018-08-28  |  2018-08-29  |  2018-08-30  |  2018-09-05  |  2018-09-06
 19 |        1.66  |        1.48  |        1.50  |        2.54  |        2.59
 18 |        1.44  |        1.56  |        1.60  |        2.96  |        3.00
 17 |        1.58  |        1.79  |        1.82  |        3.51  |        3.50
 16 |        2.34  |        2.37  |        2.53  |        4.55  |        4.41
 15 |        3.51  |        3.58  |        3.94  |        5.64  |        5.70

2018-08-28, -29 and -30 were before this PR was deployed, the other two were after it was deployed (in the version from Sept. 03).
Now this is not perfect because of course the tiles rendered every day aren't the same, but I think you can see the trend: While the performance impact is no longer horrible, there will be a noticeable perfomance impact in the higher zoom levels.

[imagico]

If such a difference is directly due to style code changes it should be measurable in a single tile test render as well. If that is the case it would also be possible to determine what the cause of the difference is which is essential to determine if something can be done about it.

[kocio-pl]

Do you think Mapnik implementation might not have this problem? ~2x slower rendering will be noticeable.

[sommerluk]

Do you think Mapnik implementation might not have this problem?

I don’t know. I would guess that we have maybe three factors that influence rendering performance here:

1. The fact that we have more attachments that before.
2. The fact that we have more SVG geometries to render. (The pattern is quite fine-graded, maybe that influences performance.)
3. The fact that we use now comp-op. (My original hope was that comp-op has no [important] impact because dst-out is similar to the normal rendering, just the other way around. But I’m not sure anymore about that.)

Number 1 and number 3 would be eliminated when using a native Mapnik implementation. Number 2 will obviously still be present also with a native Mapnik implementation (but when number 1 is eliminated, this will maybe reduce the number of SVG geometries and therefore will make that number 2 has less impact.)

Obviously I’m not sure about all this because currently there is no Mapnik release that implements this, so I have no way to test it.

[sommerluk]

@rrzefox Do you have an idea what could be the reason for this performance decrease? In the previous performance test (“simple benchmarks”) you got roughly the same performance. Was this on a per-tile base? What could cause the “big” test for 24 hours give different results from the “small” test for a few tiles?

[rrzefox]

I do not have an idea what exactly causes the performance decrease except the obvious (drawing more and querying the DB more means it'll get slower).

The 'simple benchmarks' were just that, more of a first sanity check than a proper benchmark - I rendered a few tiles to see if there was an extreme performance hit, like there was in the first attempt of this PR. Notice however that those were on Z8 - Z12 and on tiles that would already take "bloody ages" to render. "it takes on average 1 second more to render" isn't really visible on tiles that render for two minutes - but it is on high-zoom tiles that only take 1-3 seconds.

[talaj]

My original hope was that comp-op has no [important] impact because dst-out is similar to the normal rendering, just the other way around. But I’m not sure anymore about that.

Rendering a line with dst-out is just as fast as rendering a line with default composition src-over, but there are many polygons with dst-over where every pixel of rendered image needs to be composited. I was looking to the resulting XML style and there is 49 *-fill-pattern styles.

Do you think Mapnik implementation might not have this problem? ~2x slower rendering will be noticeable.

No, native Mapnik implementation will perform similarly to solid line rendering.

[sommerluk]

@talaj Thanks for commenting here!

---

there are many polygons with dst-over where every pixel of rendered image needs to be composited. I was looking to the resulting XML style and there is 49 *-fill-pattern styles.

When there would be a *-fill-pattern styles that does not match any actual geometry in the SQL data, would the slow dst-over operation be executed nevertheless for this *-fill-pattern styles, or would this *-fill-pattern styles simply be ignored without any rendering operation at all?

The reason for asking this is that currently, the SQL code generates a global bounding box for each road type – always, independent of if this particular road type really occurs in this particular tile. This could be optimized maybe…

---

native Mapnik implementation will perform similarly to solid line rendering.

What could be an approximative time frame for a Mapnik release that ships this implementation?

---

@imagico Better revert this PR for the moment?

[talaj]

@sommerluk

When there would be a *-fill-pattern styles that does not match any actual geometry in the SQL data, would the slow dst-over operation be executed nevertheless for this *-fill-pattern styles, or would this *-fill-pattern styles simply be ignored without any rendering operation at all?

The dst-over is not executed if the polygon is not rendered. It would most likely help to render it only if there are corresponding roads rendered.

What could be an approximative time frame for a Mapnik release that ships this implementation?

I can finish the feature let's say in two weeks, but what will continue is hard to guess.

[kocio-pl]

@sommerluk You can also ask OSMF admins, since this is our primary deployment target.

[sommerluk]

@tomhughes Suppose that what @rrzefox measured would be bad for OSMF servers?

[tomhughes]

Well is that CPU time in mapnik? or is a it a real time delay caused by waiting for I/O? or some combination?

[rrzefox]

It's not CPU time, it's time from "tirex starts to render the tile" to "tirex finished rendering the tile". Where that time is really lost I cannot tell, probably a combination of "more waiting for the DB to reply due to more queries" and "more CPU usage".
I don't really think there is anything that can be done here but trial and error. Deploy and prepare to revert if the effects are too severe. I'd put a warning into the changelog.

[tomhughes]

Well CPU time in postges is still CPU time ;-)

My thought was mostly that the production servers will probably handle increased I/O better than increased CPU but even that is only really a guess.

[sommerluk]

Do you think it can be deployed?