-
Notifications
You must be signed in to change notification settings - Fork 157
turning json option into macro parameter #308
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
Nice! Also the code definitely more cache friendly, it's shown by more constant time results on output. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_string
function. 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