[redux] gzip decompression inside vtquery #95

springmeyer · 2018-06-26T23:29:23Z

#73 got stale and was hard to merge after a few other things landed (like #89), so I've re-created the PR from scratch. As a next step would love PR review from @alliecrevier.

sssoleileraaa

Maybe we could chat in person about your use of emplace_back and buffers, otherwise looks good to me!

sssoleileraaa · 2018-06-27T00:12:23Z

src/vtquery.cpp

@@ -28,9 +30,12 @@ const char* getGeomTypeString(int enumVal) {
    return GeomTypeStrings[enumVal];
 }

+using materialized_prop_type = std::pair<std::string, mapbox::feature::value>;


+1 for using std::pair here. What's supposed to be stored here? materialized_prop_type makes me think a name value pair representing a type of property. Also just curious why you use the term "materialized"?

Looks like your comments later on in this PR answer my second question: de2e131#diff-d293222b917c4d9edf75c1288538554fR375

Yes, that comment is the intended documentation for this. Basically I think we need to copy the data to safely use it outside of the scope of the std::string that represents the original decompressed tile buffer. I used materialized as a fancy way to describe this copy. Other terms could be standalone_prop_type or persistent_prop_type or something.

[nitpick] Since T const& instead of const T& is the style used throughout this code file, it might make sense to use this style for pointers as well. So instead of const char* and static const char* you would use char const* and static char const* respectively. It looks weird to me but it's consistent!

@alliecrevier not quite sure which chunk of code this comment refers to. Perhaps you meant to place against another line?

Ah must be

vtquery/src/vtquery.cpp

Lines 28 to 29 in f98b2b6

static const char* GeomTypeStrings[] = {"point", "linestring", "polygon", "unknown"};

const char* getGeomTypeString(int enumVal) {

(which I could not see in the github comment display since it only shows the first couple lines above the comment).

I'm going to not touch this code right now since the differences are a little above my knowledge to feel comfortable with. Let's discuss in a ticket: #96

sssoleileraaa · 2018-06-27T00:13:17Z

src/vtquery.cpp

-
-    auto val = vtzero::convert_property_value<mapbox::feature::value, mapbox::vector_tile::detail::property_value_mapping>(property.value());
-    mapbox::util::apply_visitor(property_value_visitor{properties_obj, std::string(property.key())}, val);
+    mapbox::util::apply_visitor(property_value_visitor{properties_obj, property.first}, property.second);


sssoleileraaa · 2018-06-27T00:49:22Z

src/vtquery.cpp

+                feature.properties_vector_materialized.reserve(feature.properties_vector.size());
+                for (auto const& property : feature.properties_vector) {
+                    auto val = vtzero::convert_property_value<mapbox::feature::value, mapbox::vector_tile::detail::property_value_mapping>(property.value());
+                    feature.properties_vector_materialized.emplace_back(std::string(property.key()), std::move(val));


Should you use push_back here or do you prefer emplace_back for all cases?

Great question, this was has always been confusing to me. My sense is that both work, but one will be slightly faster if it can reduce allocations. The hope here is that emplace_back forwards the arguments such that they are not allocated once and then moved. But in reading about this in the past I've not felt 100% confident in this assertion and it may depend on the C++ version and library. Seems like https://abseil.io/tips/112 is relevant here.

sssoleileraaa · 2018-06-27T01:01:57Z

src/vtquery.cpp

+            gzip::Decompressor decompressor;
+            std::string uncompressed;
+            std::vector<std::string> buffers;
+            std::vector<std::tuple<vtzero::vector_tile, std::int32_t, std::int32_t, std::int32_t>> tiles;


int32_t tells me things would break if 64 bits were used for an int is that a correct assumption?

Great question. How about you ticket this question separately because I've been feeling like there may be some loose ends here. I've not fully digested #88 and #89 and we should probably recap if those were correct and if anything remains to be more careful about.

sssoleileraaa · 2018-06-27T01:02:30Z

src/vtquery.cpp

+            std::string uncompressed;
+            std::vector<std::string> buffers;
+            std::vector<std::tuple<vtzero::vector_tile, std::int32_t, std::int32_t, std::int32_t>> tiles;
+            tiles.reserve(data.tiles.size());


sssoleileraaa · 2018-06-27T01:04:40Z

src/vtquery.cpp

+                    buffers.emplace_back(std::move(uncompressed));
+                    tiles.emplace_back(vtzero::vector_tile(buffers.back()), tile_obj.z, tile_obj.x, tile_obj.y);
+                } else {
+                    tiles.emplace_back(vtzero::vector_tile(tile_obj.data), tile_obj.z, tile_obj.x, tile_obj.y);


why don't you do tile_obj.data.data() here like you did above?

Because tile_obj.data is a protozero::data_view (and vtzero::data_view is the same thing because of https://github.com/mapbox/vtzero/blob/3484299f270d3f1b3f8233bfbb7f3321ab7d1c0d/include/vtzero/types.hpp#L43). So in this case vtzero's vector_tile constructor is fancy enough to know about this type and has a custom constructor to save us the typing (this one https://github.com/mapbox/vtzero/blob/3484299f270d3f1b3f8233bfbb7f3321ab7d1c0d/include/vtzero/vector_tile.hpp#L67-L70). But gzip-hpp is not so smart (because we did not want to depend directly on protozero in gzip-hpp to keep it light).

Overall a string_view or data_view concept is so useful I think we should plan to either:

🍒 break out the data_view implemention in protozero and start using it everywhere. Things like gzip-hpp would then have a new dep

🍎 use someone else's string view like https://abseil.io/tips/1

🍊 or wait until we can use C++17 everywhere and use https://en.cppreference.com/w/cpp/header/string_view

@joto has been thinking that 🍊 is ideal and that is the reason that protozero has https://github.com/mapbox/protozero/blob/a44efc34e5a86e93c06390aa19c89f8e115971f6/include/protozero/data_view.hpp#L29. The idea is that you can use protozero's data view until you want to opt-into using the standard, if you have it.

sssoleileraaa · 2018-06-27T01:14:02Z

src/vtquery.cpp

+                if (gzip::is_compressed(tile_obj.data.data(), tile_obj.data.size())) {
+                    decompressor.decompress(uncompressed, tile_obj.data.data(), tile_obj.data.size());
+                    buffers.emplace_back(std::move(uncompressed));
+                    tiles.emplace_back(vtzero::vector_tile(buffers.back()), tile_obj.z, tile_obj.x, tile_obj.y);


why is buffers needed as midpoint? why can't you do tiles.push_back(vtzero::vector_tile(uncompressed))? I think I might be missing some kind of optimization reasoning behind this.

You've noticed the critical thing here. So, vtzero is written with top performance in mind. This means that it cannot/does not/refuses to accept the cost of copying or re-allocation internally. Instead, by design, it expects that the memory you pass it will stay alive for as long as vtzero needs to work on it. What this means is that the vtzero calls at

vtquery/src/vtquery.cpp

Lines 381 to 382 in de2e131

auto val = vtzero::convert_property_value<mapbox::feature::value, mapbox::vector_tile::detail::property_value_mapping>(property.value());

feature.properties_vector_materialized.emplace_back(std::string(property.key()), std::move(val));

need the buffer passed to the original vtzero::vector_tile object to still be alive. Otherwise they will reference dangling pointers internally. While this is dangerous, its by design for top performance. As long as the data passed to vtzero stays in scope (and is controlled outside of vtzero) we can have perfectly correct code and top performance without copies.

So, the std::vector<std::string> buffers; is my best attempt to deliver vtzero this promise. The std::vector<std::string> buffers; is at the correct scope such that the data it holds is perfectly alive and in memory for as long as vtzero needs it. More info at mapbox/vtzero#36 and mapbox/vtzero@a42889d

sssoleileraaa · 2018-06-27T17:26:16Z

src/vtquery.cpp

+                    auto val = vtzero::convert_property_value<mapbox::feature::value, mapbox::vector_tile::detail::property_value_mapping>(property.value());
+                    feature.properties_vector_materialized.emplace_back(std::string(property.key()), std::move(val));
+                }
+            }
        } catch (const std::exception& e) {


[nitpick] Since T const& instead of const T& is the style used throughout this code file, it might make sense to use this style for exceptions as well.

Nice 👁 . Fixed in f98b2b6

springmeyer · 2018-06-27T19:01:38Z

Thanks for your review and approval @alliecrevier. Just merged and then realized I missed tests, so created #97

re-implement #73 on top of latest master

de2e131

springmeyer added this to the 0.2.2 milestone Jun 26, 2018

springmeyer requested a review from sssoleileraaa June 26, 2018 23:29

springmeyer mentioned this pull request Jun 26, 2018

implement gzip decompression in vtquery #73

Closed

clang-format fixes

ee1bd3d

sssoleileraaa reviewed Jun 27, 2018

View reviewed changes

use consistent const& style

f98b2b6

sssoleileraaa approved these changes Jun 27, 2018

View reviewed changes

springmeyer merged commit 378659e into master Jun 27, 2018

springmeyer deleted the gzip-decode branch June 27, 2018 19:00

springmeyer mentioned this pull request Jun 27, 2018

Tests for internal decompression support #97

Closed

sssoleileraaa mentioned this pull request Jun 27, 2018

when to use std::int32_t and std::int64_t #98

Open

This was referenced Jul 1, 2018

decompress tiles using gzip-hpp #52

Closed

vtzero::convert_property_value inside worker #82

Closed

remove explicit linking to libz #101

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[redux] gzip decompression inside vtquery #95

[redux] gzip decompression inside vtquery #95

springmeyer commented Jun 26, 2018

sssoleileraaa left a comment

sssoleileraaa Jun 27, 2018

sssoleileraaa Jun 27, 2018

springmeyer Jun 27, 2018

sssoleileraaa Jun 27, 2018

springmeyer Jun 27, 2018

springmeyer Jun 27, 2018

springmeyer Jun 27, 2018

sssoleileraaa Jun 27, 2018

sssoleileraaa Jun 27, 2018

springmeyer Jun 27, 2018

sssoleileraaa Jun 27, 2018

springmeyer Jun 27, 2018

sssoleileraaa Jun 27, 2018

sssoleileraaa Jun 27, 2018

springmeyer Jun 27, 2018

sssoleileraaa Jun 27, 2018

springmeyer Jun 27, 2018

sssoleileraaa Jun 27, 2018

springmeyer Jun 27, 2018

springmeyer commented Jun 27, 2018

	static const char* GeomTypeStrings[] = {"point", "linestring", "polygon", "unknown"};
	const char* getGeomTypeString(int enumVal) {

	auto val = vtzero::convert_property_value<mapbox::feature::value, mapbox::vector_tile::detail::property_value_mapping>(property.value());
	feature.properties_vector_materialized.emplace_back(std::string(property.key()), std::move(val));

[redux] gzip decompression inside vtquery #95

[redux] gzip decompression inside vtquery #95

Conversation

springmeyer commented Jun 26, 2018

sssoleileraaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

springmeyer commented Jun 27, 2018