Update string constants to avoid global constructors - Option 1 #2693
Conversation
…bal-constructors compiler arguments enabled, using string_view for string constants to avoid global constructor/destructors.
|
|
||
| /// Set the value string of the given attribute. | ||
| void setAttribute(const string& attrib, const string& value); | ||
| void setAttribute(std::string_view attrib, const string& value); |
There was a problem hiding this comment.
Why not string_view value as well?
There was a problem hiding this comment.
just trying to make as minimal API change as possible - I think there's a good improvement to move to more string_view based API everywhere - but here (And in the other PR) I'm just trying to explore a strategy to avoid global constructor/destructor, moving the const string to string_view - it seemed to make the most sense to change the API here to accept a string_view for the attribute name.
There was a problem hiding this comment.
I'm a huge fan of string_view.
There was a problem hiding this comment.
I hear ya :)
I'm much more familiar with the Shader Gen modules than I am the Core and the requirements there to maintain compatibility.
I do know that string operations from the core are measurable impacts from a performance perspective while loading and querying the MaterialX document, so I to think it would be good for someone to take a closer look.
|
I see you have put in safety wrappers for Python (pybind). This is also necessary for Javascript (emscripten) bindings since for both the strings are temporary (in temp buffers) which are destroyed after calls and future access will be to invalid memory. If we do this, I guess there are two ways I can think of handling this: Keep the "safe" API for public API bindings and directly call them, or use wrappers. For either it needs to be simple and made required somehow to avoid possible regressions or future issues. Maybe write wrapper templates ? I'm not sure if build or analysis tools catch this, but AFAIK no public Python/JS can expose string_view directly. I think either way the performance should not change, but it would be good to regression test this. |
|
This is why I wanted to post the two possible options - to explore what concerns people maybe have for each approach. I certainly haven't kicked the tires here at all for either of the bindings - we don't really use them. (Note : I didn't touch the python bindings - because there was no CI error there - only the JS bindings) |
3 participants
As part of the work to make adopting MaterialX easier for us and lowering the number of internal patches we have to carry for security and performance reasons, this PR proposes a method we could use to avoid the global constructor/destructor calls for std::string constants.
There are actually two different possible approaches here - the other is in #2694
This PR only applies the change to a single source file
Property.cpp/h, but if/when one of the two options is selected it would be my intention to apply the same pattern at least to the rest ofMaterialXCore, and then eventually elsewhere.This approach is probably my preferred one, it uses
constexpr std::string_viewinstead ofconst std::string, but it does need API change to the attribute interface onElement. Everything compiles, so I believe it to be compatible with the existing source. Also some changes were necessary in the Javascript bindings, which I'm not familiar with - hoping experts there can highlight any problems, if they exist.