avoid malloc for Attribute with long strings

After: ``` real 1m8.124s user 16m6.620s sys 0m16.808s ``` Looks like we're solidly into diminishing returns at this point.
systemed · Dec 4, 2023 · 13b3465 · 13b3465
1 parent 3922170
commit 13b3465
Show file tree

Hide file tree

Showing 5 changed files with 97 additions and 18 deletions.
diff --git a/include/osm_lua_processing.h b/include/osm_lua_processing.h
@@ -33,6 +33,20 @@ 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.
 	
@@ -151,11 +165,8 @@ class OsmLuaProcessing {
 	void LayerAsCentroid(const std::string &layerName);
 
 	// 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 AttributeWithMinZoom(const std::string &key, const PossiblyKnownTagValue& val, const char minzoom);
 	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 MinZoom(const double z);
 	void ZOrder(const double z);

diff --git a/include/tag_map.h b/include/tag_map.h
@@ -31,9 +31,13 @@ class TagMap {
 	const std::string* getTag(const std::string& key) const;
 
 	// Return -1 if key not found, else return its keyLoc.
-	int64_t getTag(const char* key, size_t size) const;
+	int64_t getKey(const char* key, size_t size) const;
+
+	// Return -1 if value not found, else return its keyLoc.
+	int64_t getValue(const char* key, size_t size) const;
 
 	const std::string* getValueFromKey(uint32_t keyLoc) const;
+	const std::string* getValue(uint32_t valueLoc) const;
 
 	boost::container::flat_map<std::string, std::string> exportToBoostMap() const;
 

diff --git a/src/osm_lua_processing.cpp b/src/osm_lua_processing.cpp
@@ -36,7 +36,7 @@ template<>  struct kaguya::lua_type_traits<KnownTagKey> {
 		size_t size = 0;
 		const char* buffer = lua_tolstring(l, index, &size);
 
-		int64_t tagLoc = osmLuaProcessing->currentTags->getTag(buffer, size);
+		int64_t tagLoc = osmLuaProcessing->currentTags->getKey(buffer, size);
 
 		if (tagLoc >= 0) {
 			rv.found = true;
@@ -52,6 +52,49 @@ template<>  struct kaguya::lua_type_traits<KnownTagKey> {
 	}
 };
 
+template<>  struct kaguya::lua_type_traits<PossiblyKnownTagValue> {
+	typedef PossiblyKnownTagValue get_type;
+	typedef const PossiblyKnownTagValue& push_type;
+
+	static bool strictCheckType(lua_State* l, int index)
+	{
+		return lua_type(l, index) == LUA_TSTRING;
+	}
+	static bool checkType(lua_State* l, int index)
+	{
+		return lua_isstring(l, index) != 0;
+	}
+	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);
+		return rv;
+	}
+	static int push(lua_State* l, push_type s)
+	{
+		throw std::runtime_error("Lua code doesn't know how to use PossiblyKnownTagValue");
+	}
+};
+
 std::string rawId() { return osmLuaProcessing->Id(); }
 bool rawHolds(const KnownTagKey& key) { return key.found; }
 const std::string& rawFind(const KnownTagKey& key) {
@@ -128,15 +171,15 @@ 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 PossiblyKnownTagValue& val) { osmLuaProcessing->AttributeWithMinZoom(key, val, 0); },
+			[](const std::string &key, const PossiblyKnownTagValue& val, const char minzoom) { osmLuaProcessing->AttributeWithMinZoom(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) { osmLuaProcessing->AttributeNumericWithMinZoom(key, val, 0); },
 			[](const std::string &key, const float val, const char minzoom) { osmLuaProcessing->AttributeNumericWithMinZoom(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) { osmLuaProcessing->AttributeBooleanWithMinZoom(key, val, 0); },
 			[](const std::string &key, const bool val, const char minzoom) { osmLuaProcessing->AttributeBooleanWithMinZoom(key, val, minzoom); }
 	);
 
@@ -541,22 +584,23 @@ void OsmLuaProcessing::Accept() {
 }
 
 // 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) {
-	if (val.size()==0) { return; }		// don't set empty strings
+void OsmLuaProcessing::AttributeWithMinZoom(const string &key, const PossiblyKnownTagValue& val, const char minzoom) {
+	const std::string* str = &val.fallback;
+	if (val.found)
+		str = currentTags->getValue(val.index);
+
+	if (str->size()==0) { return; }		// don't set empty strings
 	if (outputs.size()==0) { ProcessingError("Can't add Attribute if no Layer set"); return; }
-	attributeStore.addAttribute(outputs.back().second, key, val, minzoom);
+	attributeStore.addAttribute(outputs.back().second, key, *str, 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) {
 	if (outputs.size()==0) { ProcessingError("Can't add Attribute if no Layer set"); return; }
 	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) {
 	if (outputs.size()==0) { ProcessingError("Can't add Attribute if no Layer set"); return; }
 	attributeStore.addAttribute(outputs.back().second, key, val, minzoom);

diff --git a/src/tag_map.cpp b/src/tag_map.cpp
@@ -90,7 +90,7 @@ const std::string* TagMap::getTag(const std::string& key) const {
 	return nullptr;
 }
 
-int64_t TagMap::getTag(const char* key, size_t size) const {
+int64_t TagMap::getKey(const char* key, size_t size) const {
 	// Return -1 if key not found, else return its keyLoc.
 	std::size_t hash = hashString(key, size);
 
@@ -107,11 +107,32 @@ int64_t TagMap::getTag(const char* key, size_t size) const {
 	return -1;
 }
 
+int64_t TagMap::getValue(const char* value, size_t size) const {
+	// Return -1 if value not found, else return its valueLoc.
+	std::size_t hash = hashString(value, size);
+
+	const uint16_t shard = hash % values.size();
+	for (int i = 0; i < values[shard].size(); i++) {
+		const std::string& candidate = *values[shard][i];
+	  if (candidate.size() != size)
+			continue;
+
+		if (memcmp(candidate.data(), value, size) == 0)
+			return shard << 16 | i;
+	}
+
+	return -1;
+}
+
 const std::string* TagMap::getValueFromKey(uint32_t keyLoc) const {
 	const uint32_t valueLoc = key2value[keyLoc >> 16][keyLoc & 0xFFFF];
 	return values[valueLoc >> 16][valueLoc & 0xFFFF];
 }
 
+const std::string* TagMap::getValue(uint32_t valueLoc) const {
+	return values[valueLoc >> 16][valueLoc & 0xFFFF];
+}
+
 boost::container::flat_map<std::string, std::string> TagMap::exportToBoostMap() const {
 	boost::container::flat_map<std::string, std::string> rv;
 

diff --git a/src/tilemaker.cpp b/src/tilemaker.cpp
@@ -392,7 +392,6 @@ int main(int argc, char* argv[]) {
 	}
 
 	// ----	Write out data
-//	return 0; // TODO
 
 	// If mapsplit, read list of tiles available
 	unsigned runs=1;