-
Notifications
You must be signed in to change notification settings - Fork 239
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
Conversation
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
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 Timings/memory for GB compared to current master (with protozero merged):
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. |
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 |
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! |
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. |
...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.
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.