Skip to content
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

add patch_inplace function #3581

Merged
merged 3 commits into from
Jul 21, 2022
Merged

add patch_inplace function #3581

merged 3 commits into from
Jul 21, 2022

Conversation

wolfv
Copy link
Contributor

@wolfv wolfv commented Jul 15, 2022

I am trying to patch a >100Mb JSON file and it turns out that the copying / destruction of JSON objects takes quite long.

Not allocating a new JSON for each patch helps a lot in terms of speed (>100x improvement).


Pull request checklist

Read the Contribution Guidelines for detailed information.

  • Changes are described in the pull request, or an existing issue is referenced.
  • The test suite compiles and runs without error.
  • Code coverage is 100%. Test cases can be added by editing the test suite.
  • The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header file single_include/nlohmann/json.hpp. The whole process is described here.

Please don't

  • The C++11 support varies between different compilers and versions. Please note the list of supported compilers. Some compilers like GCC 4.7 (and earlier), Clang 3.3 (and earlier), or Microsoft Visual Studio 13.0 and earlier are known not to work due to missing or incomplete C++11 support. Please refrain from proposing changes that work around these compiler's limitations with #ifdefs or other means.
  • Specifically, I am aware of compilation problems with Microsoft Visual Studio (there even is an issue label for these kind of bugs). I understand that even in 2016, complete C++11 support isn't there yet. But please also understand that I do not want to drop features or uglify the code just to make Microsoft's sub-standard compiler happy. The past has shown that there are ways to express the functionality such that the code compiles with the most recent MSVC - unfortunately, this is not the main objective of the project.
  • Please refrain from proposing changes that would break JSON conformance. If you propose a conformant extension of JSON to be supported by the library, please motivate this extension.
  • Please do not open pull requests that address multiple issues.

@wolfv wolfv requested a review from nlohmann as a code owner July 15, 2022 21:15
@coveralls
Copy link

coveralls commented Jul 15, 2022

Coverage Status

Coverage remained the same at 100.0% when pulling fe31d31 on wolfv:inplace_patch into d4daaa8 on nlohmann:develop.

@falbrechtskirchinger
Copy link
Contributor

falbrechtskirchinger commented Jul 16, 2022

A couple of things:

  • Please run make amalgamate (see Contribution Guidelines).
  • I think patch_inplace would be the better name, so it appears next to patch in a sorted list.
  • The documentation needs to be updated. I suggest you move docs/mkdocs/docs/api/basic_json/patch.md to patch_inplace.md and add a new patch.md, explaining patch() in terms of patch_inplace() and linking to patch_inplace.md for details. Example files need to be duplicated and adjusted.

(Separately, we should address the quite frequent failure of MinGW in CI. Can we rehost the MinGW package without violating the license?)

@wolfv wolfv force-pushed the inplace_patch branch 2 times, most recently from 604ea2d to 1e2556f Compare July 16, 2022 11:49
@wolfv
Copy link
Contributor Author

wolfv commented Jul 16, 2022

@falbrechtskirchinger thanks for the notes. What do you think about keeping docs in a single document? Otherwise there would be quite some duplication.

This really drastically improves the performance for our use case as copying and destroying a 100Mb json file is really slow (on the tune of ~1 second per patch, vs a couple nanoseconds without the copy).

I wonder if the destruction could be optimized in #3583

@falbrechtskirchinger
Copy link
Contributor

falbrechtskirchinger commented Jul 16, 2022

@falbrechtskirchinger thanks for the notes. What do you think about keeping docs in a single document? Otherwise there would be quite some duplication.

@wolfv I don't think there's precedent for similarly named functions being documented in one file and while I agree with you about the duplication, @nlohmann will probably want a dedicated patch_inplace.md file. Let's wait and see.

You're at least missing an entry in docs/mkdocs/mkdocs.yml.

- 'patch_inplace': api/basic_json/patch.md

Also, can you please prepare the examples in docs/examples and include them once a decision has been made as to where patch_inplace() should be documented?
Basically, copy patch.cpp -> patch_inplace.cpp and patch.output -> patch_inplace.output and modify patch_inplace.cpp:

    // output original document
    std::cout << std::setw(4) << doc << "\n\n";

    // apply the patch in-place
    doc.patch_inplace(patch);

    // output patched document
    std::court << std::setw(4) << doc << std::endl;

Did you run make amalgamate? You need to commit the generated single_include/nlohmann/json.hpp file as well.

@nlohmann
Copy link
Owner

@wolfv I don't think there's precedent for similarly named functions being documented in one file and while I agree with you about the duplication, @nlohmann will probably want a dedicated patch_inplace.md file. Let's wait and see.

Yes, please do so.

docs/mkdocs/docs/api/basic_json/patch_inplace.md Outdated Show resolved Hide resolved
docs/mkdocs/docs/api/basic_json/patch_inplace.md Outdated Show resolved Hide resolved
docs/mkdocs/docs/api/basic_json/patch_inplace.md Outdated Show resolved Hide resolved
@wolfv
Copy link
Contributor Author

wolfv commented Jul 21, 2022

Thanks for the review! I applied your suggestions.

@wolfv wolfv changed the title add inplace_patch function add patch_inplace function Jul 21, 2022
Copy link
Owner

@nlohmann nlohmann left a 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.

Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger left a 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.

@nlohmann nlohmann self-assigned this Jul 21, 2022
@nlohmann nlohmann added this to the Release 3.11.0 milestone Jul 21, 2022
@nlohmann
Copy link
Owner

Note to me:

  • Add new documentation to mkdocs.

@wolfv
Copy link
Contributor Author

wolfv commented Jul 21, 2022

@nlohmann thanks for approving! Is the remaining task something I should do?

@nlohmann
Copy link
Owner

Thanks for your work!

@nlohmann thanks for approving! Is the remaining task something I should do?

It's basically checking that the new Markdown file is added to docs/mkdocs/docs/api/basic_json/index.md, docs/mkdocs/mkdocs.yml, and docs/docset/docSet.sql. I'll take care of it.

@nlohmann nlohmann merged commit 09fb481 into nlohmann:develop Jul 21, 2022
@wolfv wolfv deleted the inplace_patch branch July 21, 2022 14:30
nlohmann added a commit that referenced this pull request Jul 21, 2022
nlohmann added a commit that referenced this pull request Jul 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants