[ET-VK][ez] Replace std::stringstream with std::string for Shader names
#2964
Conversation
… names ## Context Some research into efficient string concatenation suggests that streams in C++ are not quite efficient. The best way to concatenate strings seems to be creating a `std::string` and reserving sufficient capacity for the `std::string`. This diff deprecates the usage of `std::stringstream` when constructing kernel names in favor of using `std::string` directly. Differential Revision: [D55951475](https://our.internmc.facebook.com/intern/diff/D55951475/) [ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/2964
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 3faf442 with merge base 99c4f4e ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
… names ## Context Some research into efficient string concatenation suggests that streams in C++ are not quite efficient. The best way to concatenate strings seems to be creating a `std::string` and reserving sufficient capacity for the `std::string`. This diff deprecates the usage of `std::stringstream` when constructing kernel names in favor of using `std::string` directly. Differential Revision: [D55951475](https://our.internmc.facebook.com/intern/diff/D55951475/) ghstack-source-id: 221941094 Pull Request resolved: #2964
|
This pull request was exported from Phabricator. Differential Revision: D55951475 |
There was a problem hiding this comment.
I've preserved inline comments for pedagogical reasons, but there is a higher-level potentially-solvable problem here: you are building these strings just to do a lookup in a std::unordered_map whose key is std::string. If C++20 is available, you don't actually need a std::string to do that thanks to heterogeneous lookup (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0919r3.html), and instead the lookup function that VK_KERNEL_FROM_STR calls could take a std::string_view (or executorch equivalent thereof).
How high could we raise the C++ standard for the Vulkan backend?
| std::string kernel_name("binary_"); | ||
| kernel_name.reserve(kShaderNameReserve); |
There was a problem hiding this comment.
nit: you would be slightly better off reserving before writing "binary_" into the string; as written, "binary_" might get written into the inline storage once before getting copied to the heap if the compiler doesn't catch it. surprisingly, this was very easy to demonstrate: clang 17 gets it wrong because reserve() isn't getting inlined. https://godbolt.org/z/jKcf3KGG1
| apply_ndim_suffix(noop_shader_name, graph.get_val(packed).toTensor()); | ||
| apply_dtype_suffix(noop_shader_name, graph.get_val(packed).toTensor()); | ||
| return VK_KERNEL_FROM_STR(noop_shader_name.str()); | ||
| std::string noop_shader_name("no_op"); |
There was a problem hiding this comment.
this path doesn't use reserve, so on platforms without SSO this implementation could be worse. (It looks like the string data fits in 16 bytes otherwise, in which case you're good on libstdc++ and libc++, but it would be easy to break that in the future.)
| std::string kernel_name("idx_fill_texture"); | ||
| add_memory_layout_suffix(kernel_name, vten); | ||
| add_dtype_suffix(kernel_name, vten); |
There was a problem hiding this comment.
ditto missing reserve; inline size is as little as 16 bytes with libstdc++ (which I don't know if we use).
| std::string kernel_name("fill_texture__test"); | ||
| add_dtype_suffix(kernel_name, a); |
| std::stringstream kernel_name; | ||
| kernel_name << "binary_" << op_name << "_nobroadcast__test"; | ||
| apply_dtype_suffix(kernel_name, v_dst); | ||
| std::string kernel_name = "binary_" + op_name + "_nobroadcast__test"; |
There was a problem hiding this comment.
building a string with + this way isn't efficient; reserve and call append(). for the record, there are primitives for this in more modern C++ like folly::to and fmt::format and eventually std::format.
|
This pull request has been merged in a983ebc. |
Stack from ghstack (oldest at bottom):
std::stringstreamwithstd::stringfor Shader names #2964Context
Some research into efficient string concatenation suggests that streams in C++ are not quite efficient. The best way to concatenate strings seems to be creating a
std::stringand reserving sufficient capacity for thestd::string. This diff deprecates the usage ofstd::stringstreamwhen constructing kernel names in favor of usingstd::stringdirectly.Differential Revision: D55951475