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

Memory stats in RPC #1632

Merged
merged 4 commits into from Jan 30, 2019
Merged

Memory stats in RPC #1632

merged 4 commits into from Jan 30, 2019

Conversation

wezrule
Copy link
Contributor

@wezrule wezrule commented Jan 24, 2019

This provides an alternate implementation for #1439. The idea is to scan a node and all nested variables for sequence containers (std::vector, std::list etc..) and outputs the number of elements and the size in bytes of the container in a JSON-RPC call when requesting:

Request:

{
    "action":"stats",
    "type":"objects"
}

Response:

{
    "node": {
        "ledger": {
            "bootstrap_weights": {
                "count": "125",
                "size": "7000"
            }
        }
        "peers": {
            "peers": {
                "count": "38",
                "size": "7296"
            },
            "attempts": {
                "count": "95",
                "size": "3800"
            },
        },
        .
        .
        ...
    }
}

Ideally the members could be scanned recursively until something which has the methods .size () & operator[] and the value_type alias is found and then use this. However C++ currently lacks reflection (even in c++17) without some very nasty template tricks which will have problems with edge cases and not feasible to create with current time constraints. There is Boost.Hana (among others) which offer ways to add meta-data to classes and then be able to parse, but Boost.Hana has known problems with MSVC even recent 2017 versions https://blogs.msdn.microsoft.com/vcblog/2018/08/30/use-the-official-boost-hana-with-msvc-2017-update-8-compiler/. I gave it a go but ran into a few issues and it started to feel like an over-engineered solution anyway.

What I have instead done instead is implement the composite design pattern where leaf nodes are the sequence containers and composites are member variables containing sequence containers. This could have probably gone into it's own file, but I've added it to nano/lib/utility.hpp. All classes which should be processed should have a corresponding collect_seq_con_info function definition. I have not tried looking for sequence containers more than 2 levels deep, so it's possible I may have missed some, but these can be easily added.

There is only have a basic test. Full test coverage would probably require a couple dozen tests which would take a while to write, which is currently limited before the RC, and I wanted to first check if this would be accepted without spending more time on it. This should be quite self-contained and only affect the new RPC call and not have regression consequences for anything else.

I didn't want to change the interface of the existing data structures if I could help it. If I needed access to private mutexes for instance, the new function was added as a friend so that it could access it. A lot of the classes had public access specifier for the needed member variables so the majority did not need changing.

vote_uniquer/block_uniquer both had a size() method, so they somewhat act on the outside at least as a sequence container, so I have added value_type which other std containers have (although mainly because they are templates)

I noticed there was no mutex in the ledger, I don't know if that is by design.

I couple warnings about unused variables have been removed.

@wezrule wezrule requested a review from rkeene January 24, 2019 20:09
@wezrule wezrule self-assigned this Jan 24, 2019
@zhyatt zhyatt added this to the V18.0 milestone Jan 28, 2019
@zhyatt zhyatt added this to During RC in V18 Jan 28, 2019
@zhyatt zhyatt requested a review from clemahieu January 29, 2019 14:11
Copy link
Contributor

@cryptocode cryptocode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks fine to me, approving pending discussion on manual introspection vs. generating this code.

@rkeene rkeene removed their request for review January 29, 2019 15:49
@zhyatt zhyatt moved this from During RC to CP 3 (2018-01-27) in V18 Jan 30, 2019
@wezrule wezrule merged commit daba0f6 into nanocurrency:master Jan 30, 2019
@wezrule wezrule deleted the rpc_memory_stats branch February 15, 2019 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
V18
CP 3/RC 1 (2018-02-01)
Development

Successfully merging this pull request may close these issues.

None yet

3 participants