Fix Support For SystraceSection #4267
Conversation
Addresses microsoft#4245, During deforking work I left a build logic typo which caused SysTraceSection to noop. When trying to reenable in deforked React Native, we end up having a compile error since stock Facebook code will pass doubles as an arg to SysTraceSection, and we will internally only accept string. This was fixed in microsof/react-native by commenting out the SysTraceSection call that passed a double. A real solution is to allow our implementation of FbSystraceSection to accept non-string args and convert to strings, as Facebook's seemingly does. - Add a SFINAE based overload for non-string-like types to call std::to_string on them - Move functions to be private instead of public Still need a plan to validate this.
ghost
added
the
| std::bool_constant<std::is_convertible<T, std::string>::value || std::is_constructible<std::string, T>::value>; | ||
|
|
||
| template <typename Arg, typename std::enable_if_t<ConsructsToString<Arg>::value> * = nullptr> | ||
| inline void push_arg(Arg &&arg) { |
There was a problem hiding this comment.
) [](start = 32, length = 1)
noexcept?
Generally noexcept functions are 10 times smaller in binary files.
There was a problem hiding this comment.
I actually disagree about this one.
We're calling throwing code (std::array and std::to_string throw, along with the tracing stuff Anandraj added), which means we the compiler might be generating implicit catch/terminates. These are often times larger than the EH map that would be generated. I know the compiler has some liberty to avoid the implicit catch if it can reason throwing never happens, but no idea how good it is at it, and any allocation means it is unavoidable.
This is worth it if we're going to be called by noexcept code because we can pay that price in the leaf nodes to allow callers to avoid the overhead, but these functions are exclusively called by Facebook throwing APIs, so we essentially pay the price for a catch for nothing.
We've seen this issue practically in Word. Before a lot of the fixup that happened of shared libraries, moving Word's Clang build from bulk throwing to bulk noexcept increased binary size due to these implicit catches.
The way I've been thinking to approach this is:
- If we're sandwiched between throwing code, don't use noexcept. This is both to avoid implicit catch overhead and for correctness/safety.
- Try to use noexcept everywhere if we have a large object graph that we control and can ensure nothing is mixed internal to the boundary.
This whole thing is also partially mitigated by recent-ish MSVC changes that drastically reduce EH map size on x64.
Constructable was sort of wrong before, because we're actually doing assignment, but even then we likely only want things that allow implicit conversions.
|
Fixes #4245 FYI to @NickGerleman , if you use the language "Fixes" github knows to link the issue and the PR in a special way. |
|
|
I'm still having some trouble recording events, even with a WPR profile, but confirmed with a debugger we send sane things to generated EventWrite functions. Going to check this in. |
* Fix Support For SysTraceSection Addresses microsoft#4245, During deforking work I left a build logic typo which caused SysTraceSection to noop. When trying to reenable in deforked React Native, we end up having a compile error since stock Facebook code will pass doubles as an arg to SysTraceSection, and we will internally only accept string. This was fixed in microsof/react-native by commenting out the SysTraceSection call that passed a double. A real solution is to allow our implementation of FbSystraceSection to accept non-string args and convert to strings, as Facebook's seemingly does. - Use type traits to call std::to_string on args not convertible to string - Move functions to be private instead of public Still need a plan to validate this. * Simplify things a bit * Solely rely on is_convertible Constructable was sort of wrong before, because we're actually doing assignment, but even then we likely only want things that allow implicit conversions.
* Fix Support For SysTraceSection Addresses #4245, During deforking work I left a build logic typo which caused SysTraceSection to noop. When trying to reenable in deforked React Native, we end up having a compile error since stock Facebook code will pass doubles as an arg to SysTraceSection, and we will internally only accept string. This was fixed in microsof/react-native by commenting out the SysTraceSection call that passed a double. A real solution is to allow our implementation of FbSystraceSection to accept non-string args and convert to strings, as Facebook's seemingly does. - Use type traits to call std::to_string on args not convertible to string - Move functions to be private instead of public Still need a plan to validate this. * Simplify things a bit * Solely rely on is_convertible Constructable was sort of wrong before, because we're actually doing assignment, but even then we likely only want things that allow implicit conversions.
3 participants
Addresses #4245, During deforking work I left a build logic typo which caused SystraceSection to noop. When trying to re-enable in deforked React Native, we end up having a compile error since stock Facebook code will pass doubles as an arg to SysTraceSection, and we will internally only accept string.
This was fixed in microsof/react-native by commenting out the SystraceSection call that passed a double. A real solution is to allow our implementation of FbSystraceSection to accept non-string args and convert to strings, as Facebook's seemingly does.
Still need a plan to validate this. Any recommendations @mganandraj @stecrain ?
Microsoft Reviewers: Open in CodeFlow