remove PossiblyKnownTagValue

When I replaced systemed#604 with systemed#626, I botched extracting this part of the code. I had the trait, which taught kaguya how to serialize `PossiblyKnownTagValue`, but I missed updating the parameter type of `Attribute` to actually use it, so it was a no-op. This PR restores the behaviour of avoiding string copies, but now that we have protozero's data_view class, we can use that rather than our own weirdo struct.
cldellow · Jan 12, 2024 · 04a1859 · 04a1859
1 parent d749f76
commit 04a1859
Show file tree

Hide file tree

Showing 7 changed files with 34 additions and 65 deletions.
diff --git a/include/attribute_store.h b/include/attribute_store.h
@@ -10,6 +10,7 @@
 #include <boost/functional/hash.hpp>
 #include <boost/container/flat_map.hpp>
 #include <vector>
+#include <protozero/data_view.hpp>
 #include "pooled_string.h"
 #include "deque_map.h"
 
@@ -126,7 +127,7 @@ struct AttributePair {
 
 	void ensureStringIsOwned();
 
-	static bool isHot(const std::string& keyName, const std::string& value) {
+	static bool isHot(const std::string& keyName, const protozero::data_view value) {
 		// Is this pair a candidate for the hot pool?
 
 		// Hot pairs are pairs that we think are likely to be re-used, like
@@ -139,7 +140,8 @@ struct AttributePair {
 
 		// Only strings that are IDish are eligible: only lowercase letters.
 		bool ok = true;
-		for (const auto& c: value) {
+		for (size_t i = 0; i < value.size(); i++) {
+			const auto c = value.data()[i];
 			if (c != '-' && c != '_' && (c < 'a' || c > 'z'))
 				return false;
 		}
@@ -404,7 +406,7 @@ struct AttributeStore {
 	void reportSize() const;
 	void finalize();
 
-	void addAttribute(AttributeSet& attributeSet, std::string const &key, const std::string& v, char minzoom);
+	void addAttribute(AttributeSet& attributeSet, std::string const &key, const protozero::data_view v, char minzoom);
 	void addAttribute(AttributeSet& attributeSet, std::string const &key, float v, char minzoom);
 	void addAttribute(AttributeSet& attributeSet, std::string const &key, bool v, char minzoom);
 

diff --git a/include/osm_lua_processing.h b/include/osm_lua_processing.h
@@ -36,19 +36,6 @@ extern bool verbose;
 class AttributeStore;
 class AttributeSet;
 
-// A string, which might be in `currentTags` as a value. If Lua
-// code refers to an absent value, it'll fallback to passing
-// it as a std::string.
-//
-// The intent is that Attribute("name", Find("name")) is a common
-// pattern, and we ought to avoid marshalling a string back and
-// forth from C++ to Lua when possible.
-struct PossiblyKnownTagValue {
-	bool found;
-	uint32_t index;
-	std::string fallback;
-};
-
 /**
 	\brief OsmLuaProcessing - converts OSM objects into OutputObjects.
 	
@@ -183,12 +170,9 @@ class OsmLuaProcessing {
 	void LayerAsCentroid(const std::string &layerName, kaguya::VariadicArgType nodeSources);
 
 	// Set attributes in a vector tile's Attributes table
-	void Attribute(const std::string &key, const std::string &val);
-	void AttributeWithMinZoom(const std::string &key, const std::string &val, const char minzoom);
-	void AttributeNumeric(const std::string &key, const float val);
-	void AttributeNumericWithMinZoom(const std::string &key, const float val, const char minzoom);
-	void AttributeBoolean(const std::string &key, const bool val);
-	void AttributeBooleanWithMinZoom(const std::string &key, const bool val, const char minzoom);
+	void Attribute(const std::string &key, const protozero::data_view val, const char minzoom);
+	void AttributeNumeric(const std::string &key, const float val, const char minzoom);
+	void AttributeBoolean(const std::string &key, const bool val, const char minzoom);
 	void MinZoom(const double z);
 	void ZOrder(const double z);
 

diff --git a/include/pooled_string.h b/include/pooled_string.h
@@ -32,6 +32,7 @@
 #include <cstdint>
 #include <vector>
 #include <string>
+#include <protozero/data_view.hpp>
 
 namespace PooledStringNS {
   class PooledString {
@@ -40,9 +41,10 @@ namespace PooledStringNS {
       PooledString(const std::string& str);
 
 
-      // Create a std string - only valid so long as the string that is
-      // pointed to is valid.
-      PooledString(const std::string* str);
+      // Wrap a protozero::data_view - only valid so long as the
+      // data_view that is pointed to is valid; call ensureStringIsOwned
+      // if you need to persist the string.
+      PooledString(const protozero::data_view* str);
       size_t size() const;
       bool operator<(const PooledString& other) const;
       bool operator==(const PooledString& other) const;

diff --git a/src/attribute_store.cpp b/src/attribute_store.cpp
@@ -240,7 +240,7 @@ void AttributeSet::removePairWithKey(const AttributePairStore& pairStore, uint32
 	}
 }
 
-void AttributeStore::addAttribute(AttributeSet& attributeSet, std::string const &key, const std::string& v, char minzoom) {
+void AttributeStore::addAttribute(AttributeSet& attributeSet, std::string const &key, const protozero::data_view v, char minzoom) {
 	PooledString ps(&v);
 	AttributePair kv(keyStore.key2index(key), ps, minzoom);
 	bool isHot = AttributePair::isHot(key, v);

diff --git a/src/osm_lua_processing.cpp b/src/osm_lua_processing.cpp
@@ -78,9 +78,9 @@ template<>  struct kaguya::lua_type_traits<KnownTagKey> {
 	}
 };
 
-template<>  struct kaguya::lua_type_traits<PossiblyKnownTagValue> {
-	typedef PossiblyKnownTagValue get_type;
-	typedef const PossiblyKnownTagValue& push_type;
+template<>  struct kaguya::lua_type_traits<protozero::data_view> {
+	typedef protozero::data_view get_type;
+	typedef const protozero::data_view& push_type;
 
 	static bool strictCheckType(lua_State* l, int index)
 	{
@@ -92,32 +92,14 @@ template<>  struct kaguya::lua_type_traits<PossiblyKnownTagValue> {
 	}
 	static get_type get(lua_State* l, int index)
 	{
-		PossiblyKnownTagValue rv = { false, 0 };
 		size_t size = 0;
 		const char* buffer = lua_tolstring(l, index, &size);
-
-		// For long strings where we might need to do a malloc, see if we
-		// can instead pass a pointer to a value from this object's tag
-		// map.
-		//
-		// 15 is the threshold where gcc no longer applies the small string
-		// optimization.
-		if (size > 15) {
-			int64_t tagLoc = osmLuaProcessing->currentTags->getValue(buffer, size);
-
-			if (tagLoc >= 0) {
-				rv.found = true;
-				rv.index = tagLoc;
-				return rv;
-			}
-		}
-
-		rv.fallback = std::string(buffer, size);
+		protozero::data_view rv = { buffer, size };
 		return rv;
 	}
 	static int push(lua_State* l, push_type s)
 	{
-		throw std::runtime_error("Lua code doesn't know how to use PossiblyKnownTagValue");
+		throw std::runtime_error("Lua code doesn't know how to use protozero::data_view");
 	}
 };
 
@@ -203,16 +185,16 @@ OsmLuaProcessing::OsmLuaProcessing(
 	luaState["Layer"] = &rawLayer;
 	luaState["LayerAsCentroid"] = &rawLayerAsCentroid;
 	luaState["Attribute"] = kaguya::overload(
-			[](const std::string &key, const std::string& val) { osmLuaProcessing->Attribute(key, val); },
-			[](const std::string &key, const std::string& val, const char minzoom) { osmLuaProcessing->AttributeWithMinZoom(key, val, minzoom); }
+			[](const std::string &key, const protozero::data_view val) { osmLuaProcessing->Attribute(key, val, 0); },
+			[](const std::string &key, const protozero::data_view val, const char minzoom) { osmLuaProcessing->Attribute(key, val, minzoom); }
 	);
 	luaState["AttributeNumeric"] = kaguya::overload(
-			[](const std::string &key, const float val) { osmLuaProcessing->AttributeNumeric(key, val); },
-			[](const std::string &key, const float val, const char minzoom) { osmLuaProcessing->AttributeNumericWithMinZoom(key, val, minzoom); }
+			[](const std::string &key, const float val) { osmLuaProcessing->AttributeNumeric(key, val, 0); },
+			[](const std::string &key, const float val, const char minzoom) { osmLuaProcessing->AttributeNumeric(key, val, minzoom); }
 	);
 	luaState["AttributeBoolean"] = kaguya::overload(
-			[](const std::string &key, const bool val) { osmLuaProcessing->AttributeBoolean(key, val); },
-			[](const std::string &key, const bool val, const char minzoom) { osmLuaProcessing->AttributeBooleanWithMinZoom(key, val, minzoom); }
+			[](const std::string &key, const bool val) { osmLuaProcessing->AttributeBoolean(key, val, 0); },
+			[](const std::string &key, const bool val, const char minzoom) { osmLuaProcessing->AttributeBoolean(key, val, minzoom); }
 	);
 
 	luaState["MinZoom"] = &rawMinZoom;
@@ -786,25 +768,22 @@ void OsmLuaProcessing::removeAttributeIfNeeded(const string& key) {
 }
 
 // Set attributes in a vector tile's Attributes table
-void OsmLuaProcessing::Attribute(const string &key, const string &val) { AttributeWithMinZoom(key,val,0); }
-void OsmLuaProcessing::AttributeWithMinZoom(const string &key, const string &val, const char minzoom) {
+void OsmLuaProcessing::Attribute(const string &key, const protozero::data_view val, const char minzoom) {
 	if (val.size()==0) { return; }		// don't set empty strings
 	if (outputs.size()==0) { ProcessingError("Can't add Attribute if no Layer set"); return; }
 	removeAttributeIfNeeded(key);
 	attributeStore.addAttribute(outputs.back().second, key, val, minzoom);
 	setVectorLayerMetadata(outputs.back().first.layer, key, 0);
 }
 
-void OsmLuaProcessing::AttributeNumeric(const string &key, const float val) { AttributeNumericWithMinZoom(key,val,0); }
-void OsmLuaProcessing::AttributeNumericWithMinZoom(const string &key, const float val, const char minzoom) {
+void OsmLuaProcessing::AttributeNumeric(const string &key, const float val, const char minzoom) {
 	if (outputs.size()==0) { ProcessingError("Can't add Attribute if no Layer set"); return; }
 	removeAttributeIfNeeded(key);
 	attributeStore.addAttribute(outputs.back().second, key, val, minzoom);
 	setVectorLayerMetadata(outputs.back().first.layer, key, 1);
 }
 
-void OsmLuaProcessing::AttributeBoolean(const string &key, const bool val) { AttributeBooleanWithMinZoom(key,val,0); }
-void OsmLuaProcessing::AttributeBooleanWithMinZoom(const string &key, const bool val, const char minzoom) {
+void OsmLuaProcessing::AttributeBoolean(const string &key, const bool val, const char minzoom) {
 	if (outputs.size()==0) { ProcessingError("Can't add Attribute if no Layer set"); return; }
 	removeAttributeIfNeeded(key);
 	attributeStore.addAttribute(outputs.back().second, key, val, minzoom);

diff --git a/src/pooled_string.cpp b/src/pooled_string.cpp
@@ -57,10 +57,10 @@ PooledString::PooledString(const std::string& str) {
 	}
 }
 
-PooledString::PooledString(const std::string* str) {
+PooledString::PooledString(const protozero::data_view* str) {
 	storage[0] = StdString << 6;
 
-	*(const std::string**)((void*)(storage + 8)) = str;
+	*(const protozero::data_view**)((void*)(storage + 8)) = str;
 }
 
 bool PooledStringNS::PooledString::operator==(const PooledString& other) const {

diff --git a/test/pooled_string.test.cpp b/test/pooled_string.test.cpp
@@ -23,13 +23,15 @@ MU_TEST(test_pooled_string) {
 	mu_check(big.toString() != big2.toString());
 
 	std::string shortString("short");
+	protozero::data_view shortStringView = { shortString.data(), shortString.size() };
 	std::string longString("this is a very long string");
+	protozero::data_view longStringView = { longString.data(), longString.size() };
 
-	PooledString stdShortString(&shortString);
+	PooledString stdShortString(&shortStringView);
 	mu_check(stdShortString.size() == 5);
 	mu_check(stdShortString.toString() == "short");
 
-	PooledString stdLongString(&longString);
+	PooledString stdLongString(&longStringView);
 	mu_check(stdLongString.size() == 26);
 	mu_check(stdLongString.toString() == "this is a very long string");