libshvcore: Rewrite utils::joinPath #419
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I rewrote the
shv::core::utils::joinPath
function to use a fold expression instead of a recursive resolving. The intention was to improve these things:Handling of variadic template arguments is usually handled via recursion, which means that the recursive variant of
joinPath
has to be called multiple times, until it encounters the single argumentjoinPath
which only returns its argument. Using a fold expression removes recursion. Note: the number of calls to the actual implementation of thejoinPath
algorithm is the same in both versions.joinPath
with zero and one argument.This was impossible with the recursive version, because there had to be a one-arg
joinPath
overload to stop the recursion. The new version can explicitly disallow joining less than two paths and warn the user about the issue.a) As far as source code size goes, it is only negligibly shorter. The reason is that first of all, I wanted to have a nice error message when using joinPath with just 0/1 argument. This wasn't strictly necessary, but I think it's worth it.
b) The binary code might theoretically be a tiny bit smaller, because there's no more recursive template instantiation. This hasn't been tested and doesn't really matter anyway. The compile time might improve a tiny bit, but it probably won't be noticeable anyway, so I didn't bother profiling it.
Removing recursive calls to all the template functions should theoretically improve code speed. I have benchmarked most of the common usecases and the newer version is generally faster. Below are the results.
joinPath(std::string("a/cze/some/path"), std::string("some"));
https://quick-bench.com/q/iCybZk00OlP48nFncDTJiPnCd6Q
This is what I would say is the most common usage, a medium-long size path joined by a single path fragment. There's a slight improvement in Clang, and in GCC, the performance is about the same.
Clang:
GCC:
joinPath(std::string("a/cze/some/path"), std::string("/some/other/path"));
https://quick-bench.com/q/gd-mxd1qCvwndlyvQQ8RZGID8b4
This is an example of joining two longer paths. I'm not sure why exactly this happens, but it could be that the recursive calls work worse when SSO isn't in play. Also, there's quite difference between GCC and Clang, but at least the new algorithm is faster in both cases.
Clang:
GCC:
joinPath(std::string("a"), std::string("a"));
https://quick-bench.com/q/1jeJ_3LxBjMX2tuX4_Zgm4V_qOE
This is not something that's commonly used, but it shows the performance when all of the strings use SSO. Very slight improvement in both Clang and GCC.
Clang:
GCC:
joinPath(std::string("a"), std::string("a"), std::string("a"), std::string("a"), std::string("a"), std::string("a"), std::string("a"), std::string("a"), std::string("a"), std::string("a"), std::string("a"), std::string("a"));
https://quick-bench.com/q/OM59lipEuX7IdembWgmycK2_moI
Increasing the count of arguments mostly removes the speed advantage in Clang and in GCC, it actually reduces speed! Fortunately, it is slowed only by a little bit, and this example is quite contrived anyway, so it doesn't matter.
Clang:
GCC:
joinPath(std::string("some_long_path_xyz"), std::string("some_long_path_xyz"), std::string("some_long_path_xyz"), std::string("some_long_path_xyz"), std::string("some_long_path_xyz"), std::string("some_long_path_xyz"), std::string("some_long_path_xyz"), std::string("some_long_path_xyz"), std::string("some_long_path_xyz"), std::string("some_long_path_xyz"), std::string("some_long_path_xyz"), std::string("some_long_path_xyz"));
https://quick-bench.com/q/nQA0kej-AmjVRJxlzx1ocxn5X8Q
For completeness, this is a benchmark of a lot of arguments with long paths. It seems that when SSO isn't taking place, the improvement is similar in Clang and GCC, although, as in the previous benchmark, the advantage is smaller the more arguments you put in the function.
Clang:
GCC:
In conclusion, the benchmarks above showed this:
Overall, this change:
joinPath
with one or zero arguments unnecessarilyNotes:
The first version of the rewritten function was a bit simpler and didn't need the second template argument:
Unfortunately, it was slower, because it had an unnecessary copy. The final version splits the first assignment/joining and the rest, which means that for two arguments, calling the template function is esentially the same as calling the implementation. The local variable
res
gets optimized due to NRVO. It does make the code more complicated though.