[perf idea] minor performance optimization in feature building

[springmeyer]

Currently vtcomposite uses feature.for_each_property to loop over a feature's properties and add them to the builder (in the case that we are keeping a feature during overzooming).

vtcomposite/src/feature_builder.hpp

Lines 142 to 145 in 7310aea

	feature.for_each_property([&builder](vtzero::property const& p) {
	builder.add_property(p);
	return true;
	});

This can be optimized. Currently because the code works in vtzero::property objects it means - I think - that the dictionary mapping (aka table) at the layer level is fully rebuilt on the fly from those property key/values as they are added to a new layer. Rebuilding that table is expensive. It would be ideal to, instead of rebuilding the table, just use the existing index values for the key/values. The vtzero library has something called the property mapper which can make this possible: https://github.com/mapbox/vtzero/blob/master/doc/advanced.md

Using the property mapper would allow us to do:

        while (auto idxs = feature.next_property_indexes()) {
            builder.add_property(mapper_(idxs));
        }

The gocha is that feature.next_property_indexes changes the feature state (to avoid making any expensive copies of data) so to use this we'll need to pass down a non-const feature object.

[artemp]

@springmeyer - https://github.com/mapbox/vtcomposite/tree/property_mapper - it uses mutable feature reference which is not great and I only see very marginal performance increase if any. So, on a balance original implementation looks better.

[springmeyer]

@artemp in my experience it should provide a meaningful speedup when there are a lot of properties since it avoids the allocation of a hashmap for building the table. Would you be more comfortable applying the fix if vtzero gained a lambda function that could allow the feature to be used in a const way? Perhaps we could add ‘feature.for_each_property_index()’ that would return index values in the lambda arg. /cc @joto for opinions on whether this idea is viable.

[artemp]

@springmeyer - yes to const-ness ^

[joto]

Newest vtzero master now contains a feature::for_each_property_indexes() function (commit 184bb33) and the copy_properties() function of the feature builder was extended to take an optional second property_mapper argument (commit 2915725). So you can write something like this if you want to copy all properties:

        vtzero::layer_builder lbuilder{tbuilder, layer};
        vtzero::property_mapper mapper{layer, lbuilder};
        while (auto feature = layer.next_feature()) {
            vtzero::geometry_feature_builder fbuilder{lbuilder};
            fbuilder.copy_id(feature);
            fbuilder.set_geometry(feature.geometry());
            fbuilder.copy_properties(feature, mapper);
            fbuilder.commit();
        }

[springmeyer]

Thank you @joto - implemented over at #79