Concurrent instances for mapnik rebased

[ArneBab]

This pull-request recovers caching of the mapnik object by segmenting the cache by process_id and thread_id, both for seeding and for serving. To avoid delays in serving after threads are reaped, mapnik.Map(…) objects are pregenerated for the least recently used mapfile.

The segmented cache speeds up the seeding process in our setup by at least factor 4 and pregeneration of the maps reduces the delay for serving not-yet-cached map tiles is reduced from ~5 seconds to ~1 second.

Licensing under Apache license is OK.

I’d be glad if you could merge these changes and hope they prove useful!

rebase of #401 on top of current master.

[ArneBab]

In #401 (comment) I was asked to add a test for this, but I need some support:

A test fitting that could be to request map_obj() from a single instance in multiple threads and (while keeping the threads running) ensure that when the thread is different, the Map objects are never the same (not map1 is map2). Does that sound sufficient to you?

Could you give me a hint where to best locate these tests so that they fit your test structure?

Adding mapnik for the travis-ci is likely outside scope for me (and outside budget, since I don’t have the experience with configuring CI for github, so I would have too much of experimentation to do to get finished on time).

[ArneBab]

This pull-request speeds up mapnik-operations by at least factor 4, and I would love to see it merged in mapproxy so others can benefit from the improvement, too.

[ArneBab]

It’s been another 6 months now, so I’d like to ask for support again: Could you give me a hint where to best locate these tests so that they fit your test structure?

Adding mapnik for the travis-ci is likely outside scope for me since I don’t have the experience with configuring CI for github, so I would have too much of experimentation to do to get finished. Is there already a test that uses mapnik?

[justb4]

In a recent project starting to use MP with Mapnik and ran into performance issues when serving tiles. Direct Mapnik render_tile.py without MP btw was also fast, multiple tiles, keeping the Mapnik object.

So tried the mapnik.py from this PR by directly Docker-mounting, yes dirty, it into the MP install. A significant performance improvement! Though in my setup saw some GeoPackage locking errors. My setup:

• Docker Desktop for Mac (later Ubuntu) 4.3.1. Engine 20.10.11
• MapProxy 1.13.2 debian:buster-slim via Docker Image
• uwsgi 2.0.20 (64bit): MapProxy with 4 processes and 4 threads, proto=http-socket compiled with version: 8.3.0 on 12 January 2022 14:34:38 os: Linux-5.10.76-linuxkit #1 SMP Mon Nov 8 10:21:19 UTC 2021
• Source: Mapnik with OpenTopoMap style, PostGIS, GeoTIFF Dem Hillshade, cache: GeoPackage
• Cache: GeoPackage

Sometimes when flooding tile requests via uwsgi saw Geopackage Locking errors like:
FileNotFoundError: [Errno 2] No such file or directory: '/var/mapproxy/cache/tile_locks/gpkg53ff33c59ea47f3e3cc272035b380fee-67404-87968-17.lck' Traceback (most recent call last):

But never saw these errors when seeding (concurrency 8), which seemed much faster than serving via uwsgi. Running seed_only with upscale_tiles would be preferred.

So I guess the errors seen could come from a complex multiprocessing/threading interplay of uwsgi, Mapnik source and GeoPackage (SQLite) caching locking. Or maybe even Docker file-handling...Will need more time and tweaking with e.g. uwsgi processes and threads counts. But eventually will be running seed_only to max scale (18) with upscale_tiles as the area is just NL.

Anyway I hope we can move this PR forward somehow, as its gains are significant. Maybe as a config option (multithreaded: true|false) , such that the existing mapnik.py is not affected and we can start with the MT version as "experimental". There are so many configs and setups that this is very hard to test.

[weskamm]

Thanks @ArneBab for your contribution so far and sorry for the silence.
Do you think you could add a config option like @justb4 has mentioned above (e.g. similar to that in #401) ?
That way one needs to explicitly activate the new behaviour and we would keep the old behaviour as default.

Regarding the tests, you can find an existing mapnik test over here (maybe it helps):
https://github.com/mapproxy/mapproxy/blob/master/mapproxy/test/system/test_mapnik.py

[ArneBab]

Thank you for your feedback @weskamm , and I’m happy that this helps you, @justb4 ! We are currently only using this for seeding.

I don’t know yet when I’ll be able to do the config, but I have it on my todo list now (can’t do it with high priority yet). I’m trying to make the time to do this, but it might take a while.

[justb4]

Yes, I see/understand @ArneBab. Been using the mapnik.py version from this PR for months now. The locking problems I found seemed had to do with the complexity (many layers) of the Mapnik XML, complex SQL queries, Postgres optimization etc.. This has been cleared, and found no problems with the PR-version here.

Even more optimization: when developing a Mapnik style it is beneficial to immediately see results, as there are no direct tools like Tilemill here and we're not yet using CartoCSS. For this I integrated Python Watchdog in this PR's mapnik.py to listen to changes in the directory tree of Mapnik files. On change events, the Mapnik objects are reloaded. Together with special MapProxy Layers without caching (disable_storage: true) developing becomes quite convenient. Once this PR is done I can try to integrate my work in another PR, though I think 'auto-reloading' , like Mapnik Object caching, should be a config option and was bit tricky in an Multi-* context (like Thread-safety etc).

[justb4]

I realise my proposed addition will require additional dependencies (pypi Watchdog) and rigorous testing. But for me this has been a life-saver: in a development and CI/CD environment: not only caching Mapnik objects (as the purpose of this PR) but also not needing to restart MapProxy, thanks to Watchdog, triggering reloading the Mapnik config on any change. When in (map-)development phase and just saving any file in the Mapnik tree, magically reloading and seeing the effect in seconds. Or just requesting tiles over various zoom levels in different areas. It gets almost interactive then...

The codeis now public under the (ISC) license:
https://github.com/justb4/docker-mapproxy-mapserver-mapnik/tree/main/patches .
It basically takes the PR version as here and extends with Watchdog-triggered reloads. Have been running this for months without problems, at least as a development environment. In production one would use an MP cache, and upscale tiles, anyway.
I'll be happy to donate, integrate this code, but it will require an additional dependency and lots of testing. Again we may make this optional via an additional config, with some additional development. I am simply too busy, maybe someone wants to take this.

[justb4]

@weskamm I have rights to approve the workflow and merge, but you have a better understanding of MapProxy internals. Making this feature optional and rebasing is a big step forward. Once merged, I can start PR-ing my "Hot Mapnik conf reloading" (see above and here ) in a similar, i.e. making it optional, fashion. And @ArneBab thanks for patience!

[ArneBab]

I created a third branch that’s rebased where all the workflows pass.
DisyInformationssysteme#3

(though the workflows do not run the mapnik-tests, so this only shows that the change does not break something else)

Would you like me to file a PR from this here, too? Or would you prefer that I force-push into the current branch so this PR gets updated?

@justb4 thank you for getting this moving again!

[ArneBab]

I now force-pushed the rebased changes into concurrent-instances-for-mapnik-rebased.

[ArneBab]

@justb4 @weskamm please mention me here if you see more required changes to get this merged.

[weskamm]

I will have a final look now

[weskamm]

so tests still missing?

[ArneBab]

I did not create additional tests, because I did not get the existing mapnik tests to work on a clean master (without these changes — I spent far more time than expected trying to get a mapnik+seeding docker-setup working for testing all tests in test/system/test_mapnik.py) and they are skipped in the github workflow, so I would have had to write these blind.

But with the config defaulting to False, this should be safe now despite that (and the code much clearer).

Others have tested the output of seeding yesterday and today. Over the weekend we will be running a large seeding to detect potential problems.

[ArneBab]

If a test is a strict requirement (with concurrency, because that’s the only place where this can go wrong), I’ll see what I can do the next weeks, but this is a very slow process, because I have to ping-pong with those who have a full setup in which the existing mapnik tests work.

@justb4 can you check whether the speed improvements persist with the refactoring I did?

[weskamm]

Test should usually be included if possible, but i understand the troubles you have here, so it should be ok. So maybe lets wait for final feedback from @justb4 and then we can merge this in.

[ArneBab]

@justb4 do you have a chance to test this again?

[weskamm]

I am merging this now without further feedback. As its configurable and defaults to the old behaviour, we should not expect any issues for default installations. Thanks for contribution!

[ArneBab]

Thank you!

[justb4]

Very sorry, missed this completely! I can only say that I am revisiting mapnik.py (and geopackage.py) due to recent upgrades and patches to the Docker Image. I will let know via Discussions/issues/PRs here.

[ArneBab]

No worries, I know too well how easy it is to miss a PR. Thank you for the info!