Feature/ordered map O(1) #4941
Open
Feature/ordered map O(1) #4941
+1,930
−439
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Looks like you are only benchmarking random access, and not sequential access, parsing from JSON, or dumping to JSON. I would expect the current vector to have much better performance than a list for these use cases, as vector generally outperforms list by a large amount for sequential access when the containers are large. Have you also benchmarked memory usage of the containers? Can you summarize all the results in a single table rather than having them spread all over the description? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a re-implementation of ordered_map.
Besides adding a couple unit-tests the only changed "production" file is ordered_map.hpp
The new implementation uses an std::list to store the pair<key,value> entries, plus a std::unordered_map<Key*, list::iterator> for lookup.
WHAT:
The new implementation is not a subclass of vector<>neither of map<>, thus does not "automatically" expose all methods supported by them; however the following have been implemented:
WHY:
See considerations and performance tests below *.
All this starts from #3469; I proposed an approach in which we have a "json::pointer" subclass that can be used as an iterator to walk through the tree; I have the code almost ready but this code relies on three things to be "efficient":
a. json::pointer syntax/manipulation (PR #4860)
b. json::pointer object ( a very simple PR, coming soon, which simply makes json pointers internals to use a list instead of a vector, that's a four-line patch).
c. object access by "key" (This PR)
I decided to propose four separate PRs, one for each of the above and one for the "three walking pointer/itertor"; to make things clear none of these four PRs "depends" on any of the other 3 to work, it's just a matter of performance/optimization: using the forecoming tree walker without these changes can cause severe performance issues.
Code coverage (https://coveralls.io/github/nlohmann/json) remained at 100%. A test case for every new line of code.
I added a unit-ordered-map-2.cpp which SHOULD cover all the code, coverall will tell, in case it's needed I'll act properly. For the sake of safety I left the previous coverage test untouched.
If applicable, the documentation is updated.
As it's all "internal" this is probably not needed; main class has doxygen-style comments; if anything else is needed let me know.
make amalgamate.Sure.
*) Performance.
I have also added a quick unit-ordered-json-performance.cpp unit; needless to say the new implementation wins hands down on "big" objects (from hundreds of members above), the change for 64K keys in an object is orders of magnitude without noticeable impact on memory usage.
For "small" objects there is a marginal memory impact (in my calculations about 8 pointers per key in total) with an advantage in reduced memory fragmentation (appending to the tail of a vector is the root of all memory storms in C++...).
See below some comparison between "old" and "new" implementation.
Point is that there are two practical uses of this library in my experience: "document like" collections and "data"; for "document like" stuff this implementation does not change much (we are in a range where it is really hard to make performance comparisons); for "data" this makes a change, even just the current unit-unicode* unit tests would take forever with the old ordered_map stuff.
Using "inefficient" approaches (in terms of complexity) behind the scenes exposes to serious risks every time you expose a new feature; say the "three walker" thing gets in and one thinks <<Nice, let me iterate over this ten-thousand {"uuid": {object}, ...}>> and set X to Y in every object... if that's ordered_json with the current implementation the time complexity explodes.
Once again: The last PR I will propose for the three like iterator does not "depend" formally in any way from PR3469, from this one or the one I am about to post for json_iterator as std::list; nor any of these 3 depends on the othe two; it's just that I would not use the tree iterator withOUT the other 3 optimizations.