Skip to content

Conversation

@amitiuttarwar
Copy link
Owner

@amitiuttarwar amitiuttarwar commented Nov 8, 2023

patch to observe memory usage of different types of bitcoin connections

exposes RPC endpoint getpeermemoryinfo to list current memory usage for each peer

currently not included

  • TxRequestTracker
  • V2Transport
  • PeerManagerImpl things that grow in proportion to peers

- added here instead of memusage to prevent a circular dependency later
since the addrs to send get worked on & removed from the list, add the size of
the queue instead of mostly returning empty
@amitiuttarwar amitiuttarwar force-pushed the peer-memory branch 2 times, most recently from 90e268a to 90e8233 Compare November 9, 2023 04:06
Copy link
Collaborator

@mzumsande mzumsande left a comment

Choose a reason for hiding this comment

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

Nice! Just had a first look, will review in more detail / run the patch next week.
Not sure how important, but maybe m_node_states should be accounted for too (in particular vBlocksInFlight)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

does m_headers_sync->m_header_commitments need special treatment?

Copy link
Owner Author

Choose a reason for hiding this comment

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

thanks! I missed that field (comments on my attempt to understand it below)

PeerRef peer = GetPeerRef(pfrom->GetId());
if (peer == nullptr) return false;

peer->inaccessible_dyn_memusage = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't completely understand this - why is this memory inaccessible, and why do you query m_addrs_to_send and m_addr_known here, and not in PeerDynamicMemoryUsage?

Copy link
Owner Author

Choose a reason for hiding this comment

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

since the m_addrs_to_send work queue is frequently fluctuating, the idea was to try to capture it at a point in time where it's more representative. generally, ProcessMessages fills it up, and then SendMessages services / clears it. so at a random point in time, it's most likely empty. but when entering ProcessMessages for a peer, it might have some addresses on there because of calling RelayAddress from other peers.

does that help? def not the most robust solution, but I think it should be slightly better than directly calling from PeerDynamicMemoryUsage

@amitiuttarwar
Copy link
Owner Author

m_headers_sync->m_header_commitments

I believe af214a8 should accurately calculate and add the memory usage of the bitfield deque.

however, 1a8d64d was my attempt at getting some manual logging, and it's not being triggered. my next step is to understand when m_headers_sync is instantiated, so I can confirm if I'm calculating m_header_commitments properly.

these are some notes about reasoning behind thinking the first commit should properly calculate the memory usage

  • peer struct has a member m_headers_sync which is a unique_ptr to a
    headerssyncstate

  • headerssyncstate has two fields with dynamic memory usage

  1. deque<compressedheader> m_redownloaded_headers
  2. bitdeque<> m_header_commitments
  • bitdeque<>: the empty <> indicates it's invoking a template with a
    default argument. the template definition is found in src/util/bitdeque.h,
    with the arg defined as <int blobsize = 4096 * 8>. the comment on the class
    says that the blobsize indicates the up-front allocation is 4096 bytes by
    default.

  • I tried printing the size of the bitdeque, and got 56 bytes. (added to
    peermanagerimpl::initializenode under the abcdbool conditional). this
    makes sense because the memory usage is dynamic with a std::deque on the
    class.

  logprintf("abcd sizeof bitdeque - %d \n", sizeof(peer->m_headers_sync->m_header_commitments));
  • the main data structure on the bitdeque class is m_deque, which is
    defined as a std::deque<std::bitset<4096 * 8>>

  • an std::bitset is a fixed size data structure. so the
    dynamicusage(std::deque<x>) function should work for the m_deque member.

@amitiuttarwar
Copy link
Owner Author

amitiuttarwar commented Nov 11, 2023

@mzumsande

but maybe m_node_states should be accounted for too (in particular vBlocksInFlight)?

yup def, it's on my list but it's a good sign that you're catching these :)

I don't yet have calculations for any of the dynamic memory usage on PeerManagerImpl that scales with peers (mentioned in this OP)

@mzumsande
Copy link
Collaborator

my next step is to understand when m_headers_sync is instantiated

I think it's only used during the new headerssync phase right after startup (see PR 25717). You could start a node on mainnet / testnet /signet with an empty datadir - the default logging will show you the progress. I would expect the memory usage to slowly grow in the first phase (when we download headers for the first time), reach its maximum in the second phase (the re-download). After all headers are synced successfully it should be back to zero.

@mzumsande
Copy link
Collaborator

I reviewed the branch again, and it looks good, all calculations make sense to me!

I tested the headerssync calculations with a fresh headers sync on signet, only adding a log for the fields memusage::DynamicUsage(m_headers_sync->m_redownloaded_headers) and memusage::DynamicUsage(m_headers_sync->m_header_commitments.m_deque)where you calculate them - here is an excerpt:

2023-11-30T19:08:27Z MZ HS memory: 32 4144
2023-11-30T19:08:27Z Pre-synchronizing blockheaders, height: 146000 (~85.07%)
2023-11-30T19:08:27Z MZ HS memory: 32 4144
2023-11-30T19:08:28Z MZ HS memory: 128032 4144
2023-11-30T19:08:28Z MZ HS memory: 256032 4144
2023-11-30T19:08:28Z MZ HS memory: 384032 4144
2023-11-30T19:08:28Z MZ HS memory: 512032 4144
2023-11-30T19:08:28Z MZ HS memory: 640032 4144
2023-11-30T19:08:28Z MZ HS memory: 768032 4144
2023-11-30T19:08:28Z MZ HS memory: 896032 4144
2023-11-30T19:08:28Z Synchronizing blockheaders, height: 1559 (~0.91%)
2023-11-30T19:08:29Z MZ HS memory: 924256 4144
2023-11-30T19:08:29Z Synchronizing blockheaders, height: 3559 (~2.07%)
2023-11-30T19:08:29Z MZ HS memory: 924256 4144
2023-11-30T19:08:29Z Synchronizing blockheaders, height: 5559 (~3.23%)
2023-11-30T19:08:29Z MZ HS memory: 924256 4144
2023-11-30T19:08:29Z Synchronizing blockheaders, height: 7559 (~4.40%)
2023-11-30T19:08:29Z MZ HS memory: 924256 4144

m_redownloaded_headers (the cache) is empty in the pre-sync phase, then grows quickly as the cache is filling up, and stays at its capacity until headerssync is finished and m_headers_sync destroyed.

the comment on the class says that the blobsize indicates the up-front allocation is 4096 bytes by default

Yes, that makes sense, I get 4144 for memusage::DynamicUsage(m_headers_sync->m_header_commitments.m_deque).
So the numbers make sense to me!

Also, headerssync is something we only do with a single peer at a time (at least usually, if it's too slow we would add more peers) as part of IBD and the memory is cleared after it has finished, so it shouldn't be too important for blocksonly scaling.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants