-
-
Notifications
You must be signed in to change notification settings - Fork 7.3k
Optimize binary get_number implementation by reading multiple bytes at once
#4391
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
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
nlohmann
left a comment
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! BeforeAfter |
… 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_numberimplementation by reading multiple bytes at once, which saves calling overhead especially when interacting with file I/O. It addsget_elementsto input adapters and allow them to select more efficient underlying calls to read multiple bytes if availablePerformance for reading msgpack from C style
FILEdevelopbranch:
Questions:
get_elementsinwide_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: ifwcharis used, likely the content has non-ASCII text and we won't want to interpret it as binary numbers, whereget_elementsis 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/nlohmanndirectory, runmake amalgamateto create the single-header filessingle_include/nlohmann/json.hppandsingle_include/nlohmann/json_fwd.hpp. The whole process is described here.Please don't
#ifdefs or other means.