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

json destructor quite slow #3583

Open
2 tasks done
wolfv opened this issue Jul 16, 2022 · 7 comments
Open
2 tasks done

json destructor quite slow #3583

wolfv opened this issue Jul 16, 2022 · 7 comments

Comments

@wolfv
Copy link
Contributor

wolfv commented Jul 16, 2022

Description

I was running patch over a ~100Mb repodata file and noticed that it was slow due to copying and destroying the entire object multiple times.

I am quite curious if the performance of destroy() could be improved? On my computer, it takes roughly 1 second to parse the file and create the json object, but also 0.5 seconds to destroy the json object.

Reproduction steps

Download a large JSON file, such as https://conda.anaconda.org/conda-forge/linux-64/repodata.json (curl --compressed https://conda.anaconda.org/conda-forge/linux-64/repodata.json)

and run the following code:

#include <iostream>
#include <chrono>
#include <nlohmann/json.hpp>

int
main()
{
    std::ifstream rdata("repodata.json");
    std::unique_ptr<nlohmann::json> j = std::make_unique<nlohmann::json>();
    
    {
        auto t0 = std::chrono::high_resolution_clock::now();
        rdata >> (*j);
        auto t1 = std::chrono::high_resolution_clock::now();
        std::cout << "took " << std::chrono::duration_cast<std::chrono::milliseconds>(t1-t0).count() <<" ms." << std::endl;
    }

    {
        auto t0 = std::chrono::high_resolution_clock::now();
        j.reset();
        auto t1 = std::chrono::high_resolution_clock::now();
        std::cout << "took " << std::chrono::duration_cast<std::chrono::milliseconds>(t1-t0).count() <<" ms." << std::endl;
    }
}

Expected vs. actual results

destruction should have minimal runtime cost

Minimal code example

No response

Error messages

No response

Compiler and operating system

clang 12, macOS

Library version

3.10.5

Validation

@falbrechtskirchinger
Copy link
Contributor

Performance is not a strong focus of this library, so generally, the answer to "Can X be done faster?" is yes.

I halved the runtime of the destructor without too much effort, but I have to think about this more carefully before submitting a PR.

@wolfv wolfv mentioned this issue Jul 16, 2022
4 tasks
@falbrechtskirchinger
Copy link
Contributor

I'm going to wait for #3575 to be closed, as that PR touches the same functions and I don't want to deal with the inevitable merge conflicts.

As a preview, here are the respective call graphs of the current destruction implementation (destroy by flattening) and a simple and obvious optimization (destroy by iterative traversal). The number of calls is reduced from ~14e6 to ~6e6, about halving the time taken to destroy repodata.json.

01_destroy_by_flattening_cropped
02_destroy_by_iterative_traversal_cropped

@wolfv
Copy link
Contributor Author

wolfv commented Jul 17, 2022

impressive!

@gregmarr
Copy link
Contributor

As a preview, here are the respective call graphs of the current destruction implementation (destroy by flattening) and a simple and obvious optimization (destroy by iterative traversal). The number of calls is reduced from ~14e6 to ~6e6, about halving the time taken to destroy repodata.json.

Just checking, is this still preserving the non-recursive nature of the destroy operation?

@falbrechtskirchinger
Copy link
Contributor

Of course. I hate recursion with passion. :-)

@gregmarr
Copy link
Contributor

Of course. I hate recursion with passion. :-)

I assumed so based on the call counts, but I just wanted to check before you got too far down the path, because the current non-recursive implementation is due to a past stack overflow issue destroying deeply nested json objects.

@TheZoc
Copy link

TheZoc commented Nov 14, 2023

@falbrechtskirchinger Do you happen to (still?) have a branch with this modification?
I'd like to give it a try

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants