`archive_reader_binsafe::read_object_begin` performance about zenkit HOT 4 CLOSED

Generally, I wouldn't be opposed to using sscanf since it makes really easy to understand what's being parsed and a 40% performance improvement, especially in read_object_begin would be very impressive. We would not be able to use sscanf_s, though, since that's non-standard. Something like

char class_name[128];
char object_name[128];

sscanf(line.c_str(), "[%127s %127s %hd %d]", object_name, class_name, &obj.version, &obj.index);

could work, however. I'll test that out.

from zenkit.

So I've done some benchmarking. I can't quite reproduce your 40% performance improvement. Maybe it's an issue with one of the standard libraries. I am still getting about 10-15% better performance so it's probably a good idea to implement it anyways but I'm kinda confused about this difference. Here are some of my benchmark results (sadly I don't have a proper benchmark suite yet so I can't really provide you with an easy way of reproducing this):

sscanf

stringstream

Patch

This are the changes I've made (I've applied the fix to both archive_reader_ascii as well as archive_reader_binsafe):

Index: source/archive/archive_ascii.cc
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/source/archive/archive_ascii.cc b/source/archive/archive_ascii.cc
--- a/source/archive/archive_ascii.cc	(revision 21dbecd6dc6890510a8218d0aa0868b6cbdb0b14)
+++ b/source/archive/archive_ascii.cc	(date 1668629105666)
@@ -27,15 +27,21 @@
 			return false;
 
 		input.mark();
+
 		auto line = input.get_line();
+		char class_name[128];
+		char object_name[128];
 
-		if (!line.starts_with('[') || !line.ends_with(']') || line.length() <= 2) {
+		auto parsed_elements =
+		    std::sscanf(line.c_str(), "[%127s %127s %hu %u]", object_name, class_name, &obj.version, &obj.index);
+
+		if (parsed_elements != 4) {
 			input.reset();
 			return false;
 		}
 
-		std::stringstream ss {line.substr(1, line.size() - 2)};
-		ss >> obj.object_name >> obj.class_name >> obj.version >> obj.index;
+		obj.object_name = object_name;
+		obj.class_name = class_name;
 		return true;
 	}
 
Index: source/archive/archive_binsafe.cc
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/source/archive/archive_binsafe.cc b/source/archive/archive_binsafe.cc
--- a/source/archive/archive_binsafe.cc	(revision 21dbecd6dc6890510a8218d0aa0868b6cbdb0b14)
+++ b/source/archive/archive_binsafe.cc	(date 1668629090506)
@@ -41,13 +41,19 @@
 		}
 
 		auto line = input.get_string(input.get_ushort());
-		if (!line.starts_with('[') || !line.ends_with(']') || line.length() <= 2) {
+		char class_name[128];
+		char object_name[128];
+
+		auto parsed_elements =
+		    std::sscanf(line.c_str(), "[%127s %127s %hu %u]", object_name, class_name, &obj.version, &obj.index);
+
+		if (parsed_elements != 4) {
 			input.reset();
 			return false;
 		}
 
-		std::stringstream ss {line.substr(1, line.size() - 2)};
-		ss >> obj.object_name >> obj.class_name >> obj.version >> obj.index;
+		obj.object_name = object_name;
+		obj.class_name = class_name;
 		return true;
 	}

from zenkit.

I can't quite reproduce your 40% performance improvement.

Windows10, mingw, debug. Also running stuff on laptop definitely not helping
Today I've attempted to measure again - performance is all over the place unfortunately(sometimes better, sometimes worse).

While on that I have a question about read_string design:
If you have buffer with mmap or real memory behind the hood, would it make more sense to return a string_view from buffer::get_string? That might be helpful for code that doesn't store string, but parses it instead.

PS:
Reason why debug performance of load-time has value to me, is to iterate quicker on a game development.

from zenkit.

If you have buffer with mmap or real memory behind the hood, would it make more sense to return a string_view from buffer::get_string?

The idea behind buffer is, that you can supply your own backend (ie. a std::ifstream). This design necessitates copying data instead of just referring to it through a pointer. It just happens to be the case that by default, only heap and mmap backings are available:

https://github.com/lmichaelis/phoenix/blob/fdfca88edd133efa38b6fef375036c4b8fadd29d/source/buffer.cc#L12-L118

In addition to that, copying the data into a std::string makes it much safer to use and you can't accidentally refer to unmapped memory after the buffer deallocates. Before thinking about doing something like this, I'd like to explore other areas to possibly improve on.

Reason why debug performance of load-time has value to me, is to iterate quicker on a game development.

I agree. Performance is important. I'd like to avoid sacrificing memory safety for performance wherever possible though.

from zenkit.