Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use vtzero instead of libprotobuf #625

Merged
merged 3 commits into from
Dec 28, 2023
Merged

Conversation

cldellow
Copy link
Contributor

Replaces #624

Switch from libprotobuf to vtzero, and fix a few places where we copied the geometries during writing.

I expect a roughly 7% speedup based on the measurements in #624.

I don't use --merge, so my testing of the --merge mode was somewhat artificial. I built an mbtiles, then built it again with --merge and checked that features seemed duplicated.

Switch from libprotobuf to vtzero, and fix a few places where we copied
the geometries during writing.

GB, lua-interop:

real	1m51.572s
user	26m45.201s
sys	0m16.575s

GB, this branch:

real	1m43.007s
user	25m0.658s
sys	0m15.263s

I haven't looked too closely at the output tiles yet, but they seem
correct.

Still todo: revive support for --merge
@systemed systemed mentioned this pull request Dec 28, 2023
18 tasks
@systemed
Copy link
Owner

Handy speedup, plus it makes the Mac build, in particular, much simpler (since Google helpfully changed protobuf to depend on Abseil, which unleashed the usual forces of Homebrew dependency hell).

I ran a quick test with --merge with the (adjacent) Gloucestershire and Oxfordshire extracts and that looks good.

Timings/memory for GB compared to current master (with protozero merged):

master (with --lazy-geometries):
	Elapsed (wall clock) time (h:mm:ss or m:ss): 5:03.94
	Maximum resident set size (kbytes): 9133392
master (without):
	Elapsed (wall clock) time (h:mm:ss or m:ss): 4:51.17
	Maximum resident set size (kbytes): 12174424

vtzero branch (with):
	Elapsed (wall clock) time (h:mm:ss or m:ss): 4:50.95
	Maximum resident set size (kbytes): 9154964
vtzero branch (without):
	Elapsed (wall clock) time (h:mm:ss or m:ss): 4:44.34
	Maximum resident set size (kbytes): 12307348

There is a very slight uptick in RAM usage - it's probably nothing (just an artefact of whatever vtzero does) but I thought I'd mention it in case there's any chance of a memory leak. But I couldn't see anything obvious on scanning the code.

@cldellow
Copy link
Contributor Author

Thanks for measuring the memory - I was thinking I should do that, but didn't get around to it. :)

I suspect it could be explainable by how vtzero tracks the strings for key/values.

The old code maintained a vector of keys and a vector of values, and did a linear search to translate from value to index. Very memory efficient, and very fast when the total number of distinct values was less than, say, 100 items. I had noticed a few degenerate cases in the old code where a tile had > 1000 POIs and would take a few seconds to do the lookups.

By contrast, vtzero's default strategy is to make a std::map to track things. Much more consistent runtime, but with (relatively) much more memory overhead.

@systemed systemed merged commit ae1981b into systemed:master Dec 28, 2023
5 checks passed
@systemed
Copy link
Owner

That would make sense. That suggests the impact shouldn't balloon with bigger areas - it's only 0.2% anyway so nothing really to worry about. Thanks.

I'll merge this and look at simplifying the Mac install in due course!

@cldellow
Copy link
Contributor Author

Ah, this broke the docker build when merged to master. I'll see if I can figure out why, and maybe also send a PR to enable docker builds (but not pushes) in PRs.

cldellow added a commit to cldellow/tilemaker that referenced this pull request Dec 28, 2023
...but don't push

I'm hoping this will fail the docker build, vs
systemed#625, where it passed.

Once it does, then I'll try to actually fix the Docker build.
@cldellow cldellow mentioned this pull request Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants