Conversation
|
Looks good. Do you think there be a macro for if-constexpr for C++17 enabled compilers? Something like this? #if __cplusplus >= 201703L || (defined(_MSVC_LANG) && _MSVC_LANG >= 201703L)
#define FASTFLOAT_IF_CONSTEXPR17(x) if constexpr (x)
#else
#define FASTFLOAT_IF_CONSTEXPR17(x) if (x)
#endif
...
FASTFLOAT_IF_CONSTEXPR17(basic_json_fmt) |
There was a problem hiding this comment.
Could you move the #include "event_counter.h" at the top of the file outside of the ifdef? That way it will compile on Windows too.
|
Nice! Also the code definitely more cache friendly, it's shown by more constant time results on output. |
dalle
left a comment
There was a problem hiding this comment.
Looks good. I commented somewhere else that we could explore using if-constexpr, but it's up to you. I'm a bit worried of "constant expression in if-statement" warnings appearing.
|
Also about std::from_chars and compatibility, as you can remember, the standard regexp isn't ideal and the fmt library, especially with compile time parsing macros |
It's probably fine because it's old code and in the std function should be consteval in used in constexpr if. Maybe I should write a defect report for standard std::is_constant_evaluated. What do you think? |
|
I am adding #if defined(__cpp_if_constexpr) && __cpp_if_constexpr >= 201606L
#define FASTFLOAT_IF_CONSTEXPR17(x) if constexpr (x)
#else
#define FASTFLOAT_IF_CONSTEXPR17(x) if (x)
#endif |
|
Maybe it's better to convert all options into a template parameter? |
Well. That's an empirical question (i.e., see benchmarks). |
We have added a few options (such as json_fmt). These trigger a short series of branches in our
parse_number_stringfunction. Unfortunately, this is costing us a bit of performance. We can get it back by turning the runtime parameters into template parameters... there is still a branch, but it is more direct and likely extremely cheap.Building:
Let us benchmark on an Apple M2 processor (LLVM 16). These benchmarks are part of our library.
Before:
After:
On the mesh dataset, we get a performance boost of about 15%.
This might be related to PR #307 by @IRainman