Wean ourselves off of OIIO::string_view::c_str() #1458
Conversation
OIIO's string_view::c_str() was cute, but it's a little dicey and it's not present in the C++20 std::string_view. We didn't use it in a ton of places, so replace with other idioms so that in the future we are better able to interchange with or switch to std::string_view (when our minimum supported C++ standard is high enough). Signed-off-by: Larry Gritz <[email protected]>
| @@ -254,7 +254,8 @@ class OSLEXECPUBLIC ShadingSystem | |||
| return attribute (name, TypeDesc::FLOAT, &f); | |||
| } | |||
| bool attribute (string_view name, string_view val) { | |||
| const char *s = val.c_str(); | |||
| std::string valstr(val); | |||
There was a problem hiding this comment.
Will this create an extra copy for the local variable valstr?
There was a problem hiding this comment.
Yeah, it'll create a temporary std::string. That seems a little icky, but remember that what this is replacing was potentially creating a ustring, so it's probably not a lot worse.
This is called maybe a few dozen times, tops, to set up the shading system. So I don't think there is a legit perf issue here either way.
If this were called VERY frequently, I would add specializations for std::string and ustring that would avoid the temp string creation, but I'm not sure it's worth it for a method that's called a handful of times total.
There was a problem hiding this comment.
The same pattern happens a few times, but I think the concern is that the void* flavor of attribute() (which accepts a TypeDesc to say what the void* is) -- assumes a C-style null-terminated string.
So unfortunately we can't support these cases without forcing a copy from the string_view. In the future we could redefine that case to accept a pointer to a string_view, but it would be a behavior change.
On a sufficiently modern c++ standard library, short strings would at least not go through the heap.
There was a problem hiding this comment.
Ok, so string_view (OIIO or C++17) doesn't require null terminated string. So I guess the copy is necessary to ensure null termination. Does make me think that the next call down might be better to add direct support for string_view (vs. const char *) assuming it's going to make its own copy anyway.
There was a problem hiding this comment.
I could make additional overloads for ustring and/or std::string that could avoid the copy.
But these are not inner loop calls with performance implications. We're talking about at most tens of calls per execution of the program.
There was a problem hiding this comment.
For the record, currently there are only 11 documented ShadingSystem::attribute() settings whose value is a single string. Five of them are only useful for my debugging of the implementation of OSL.
This is not a performance issue.
There was a problem hiding this comment.
I agree this isn't a performance issue. Worst remotely reasonable case is if you want to change all the renderer_outputs or the shadersearchpath per shader, and even then you're only going to get in the thousands of calls, maybe up to megabytes of allocations in the most extreme situations -- and in those situations that should be dwarfed by shader compile and execute time and space.
| @@ -3912,7 +3912,7 @@ ClosureRegistry::register_closure (string_view name, int id, | |||
| * because we will be allocating the real struct inside it. */ | |||
| OSL_ASSERT_MSG(params[i].field_size <= int(alignof(ClosureComponent)), | |||
| "Closure %s wants alignment of %d which is larger than that of ClosureComponent", | |||
There was a problem hiding this comment.
Beyond the scope of this review -- but would it be possible for these macros to use the fmt::printf under the hood?
There was a problem hiding this comment.
Yes, I think so. I believe when I first wrote this macro, it may have been prior to use of the formatted output template magic, and also I was a little hesitant because fmt::printf is a complicated implementation that made me nervous about calling in the middle of a panic bad enough to cause us to assert. But maybe it would be fine.
OIIO's string_view::c_str() was cute, but it's a little dicey and it's
not present in the C++20 std::string_view. We didn't use it in a ton
of places, so replace with other idioms so that in the future we are
better able to interchange with or switch to std::string_view (when
our minimum supported C++ standard is high enough).
Signed-off-by: Larry Gritz [email protected]