-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
Optimize binary get_number
implementation by reading multiple bytes at once
#4391
Conversation
b25a53f
to
ea8b03d
Compare
🔴 Amalgamation check failed! 🔴The source code has not been amalgamated. @TianyiChen |
I like this idea! @TianyiChen any plans to continue working on this? |
fa2712d
to
2362b3f
Compare
Sure, I have rebased the branch and added a from msgpack benchmark using C FILE. |
7c459d8
to
006d23a
Compare
Update input_adapters.hpp
72b20eb
to
97fe5f2
Compare
895132a
to
7acebd4
Compare
7acebd4
to
26cddc6
Compare
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 to me.
I could reproduce the benchmark results. Good job, thank you! Before
After
|
… at once (nlohmann#4391) * multibyte binary reader * wide_string_input_adapter fallback to get_character Update input_adapters.hpp * Update json.hpp * Add from msgpack test * Test for broken msgpack with stream, address some warnings * Reading binary number from wchar as an error, address warnings * Not casting float to int, it violates strict aliasing rule
This PR improves performance for
get_number
implementation by reading multiple bytes at once, which saves calling overhead especially when interacting with file I/O. It addsget_elements
to input adapters and allow them to select more efficient underlying calls to read multiple bytes if availablePerformance for reading msgpack from C style
FILE
develop
branch:
Questions:
get_elements
inwide_string_input_adapter
, I encountered a compile error without it. If I don't implement it, tests are passing locally, which seems to make sense: ifwchar
is used, likely the content has non-ASCII text and we won't want to interpret it as binary numbers, whereget_elements
is currently used. After looking into more UTF-8 decoding rules I think parsing binary from wide string doesn't make sense, and is making it an error.Pull request checklist
Read the Contribution Guidelines for detailed information.
include/nlohmann
directory, runmake amalgamate
to create the single-header filessingle_include/nlohmann/json.hpp
andsingle_include/nlohmann/json_fwd.hpp
. The whole process is described here.Please don't
#ifdef
s or other means.