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

[update] reduce chainbase memory usage #19

Merged
merged 9 commits into from
Aug 29, 2023
Merged

[update] reduce chainbase memory usage #19

merged 9 commits into from
Aug 29, 2023

Conversation

greg7mdp
Copy link
Contributor

@greg7mdp greg7mdp commented Aug 17, 2023

[alternate from PR 14 by @taokayan, description copied]. This builds upon @taokayan's work.

Reduce offset_node_base memory usage inside the RBtree of chainbase

This PR will reduce the chainbase memory usage

old implementation:

  • _parent, _left, _right, _color consume 32 bytes together (8 bytes each).

new implementation

  • _parent, _left, _right, _color consume 16 bytes (42 + 42 + 42 + 2 bits) together.
  • use -2 (instead of 2) as erased_flag since _color is a 2 bit signed int.

limitations:

  • offset pointers _parent, _left & _right only have signed 42-bit each (so 41-bit absolute value), shifted by two bits thanks to alignment, implying a maximum of chainbase size of 2^43 = 8TB.
  • offset_node_base objects must always be allocated within the same segment due to the 42-bit offset limit. Using a stack variable of offset_node_base is no longer valid (as stack address starts from 0x7fffxxxxxxxx which is too far away from heap addresses).
  • possible minor performance impact due to misalignment and extra bit operation required. Or performance could possibly be better thanks to higher memory locality.

tested results:

using mainnet snapshot snapshot-308122667.bin.tar.gz:

  • old chainbase memory usage: 28799134736
  • memory usage in this PR: 23136280976 (around 20% reduction)

[greg 8/21/2023] retested with the current PR version and confirmed the memory savings as reported by curl -X POST http://127.0.0.1:8888/v1/db_size/get after loading snapshot.

taokayan and others added 8 commits May 5, 2023 16:25
When using 42 bit offsets as pointers in `undo_index`, the existing test using `std::allocator` was
failing (as 64 bit pointers were truncated to 42 bits). This uses `chainbase_node_allocator` (which
uses `bip::offset_ptr<T>` as pointers), which when truncated to 42 bits allow to address 2TB of memory.
@taokayan
Copy link
Contributor

taokayan commented Aug 18, 2023

since the offset values are shifted by 3 bits, is it possible to collide with the special value 1 which is re-purposed for representing nullptr?

@greg7mdp
Copy link
Contributor Author

greg7mdp commented Aug 18, 2023

since the offset values are shifted by 3 bits, is it possible to collide with the special value 1 which is re-purposed for representing nullptr?

No, I don't think so. I explained why in the comment:

   // A concern could be that `1` is a special value meaning `nullptr`. However we could not
   // get an offset of 8, since `sizeof(offset_node_base) == 16`, so we could not have
   // difference between two `node_ptr` less than 16.

Do you think I missed a possible issue?

Now I realize I didn't completely think it through. The chainbase_node_allocator allocates nodes in batches of 64, so the offsets between two node_ptr in a single batch would be zero modulo 16.
However I did not look at how these batches are allocated by the segment manager (bip::managed_mapped_file::segment_manager). If they also are addresses aligned to a 16 byte boundary, we should be good I think. I'll verify that tomorrow and update the comment.

Striking the comment above, because we store offsets between node_ptr in _parent, _left, _right and these offsets (when >> 3) can never be 1.

@taokayan
Copy link
Contributor

since the offset values are shifted by 3 bits, is it possible to collide with the special value 1 which is re-purposed for representing nullptr?

No, I don't think so. I explained why in the comment:

   // A concern could be that `1` is a special value meaning `nullptr`. However we could not
   // get an offset of 8, since `sizeof(offset_node_base) == 16`, so we could not have
   // difference between two `node_ptr` less than 16.

Do you think I missed a possible issue?

Now I realize I didn't completely think it through. The chainbase_node_allocator allocates nodes in batches of 64, so the offsets between two node_ptr in a single batch would be zero modulo 16. However I did not look at how these batches are allocated by the segment manager (bip::managed_mapped_file::segment_manager). If they also are addresses aligned to a 16 byte boundary, we should be good I think. I'll verify that tomorrow and update the comment.

Striking the comment above, because we store offsets between node_ptr in _parent, _left, _right and these offsets (when >> 3) can never be 1.

ah i see. that's a good catch. thanks!

@greg7mdp
Copy link
Contributor Author

@taokayan how did you measure memory usage showing the 20% reduction?

@taokayan
Copy link
Contributor

@taokayan how did you measure memory usage showing the 20% reduction?

you can use the db_size plugin API: http://127.0.0.1:8888/v1/db_size/get

@greg7mdp greg7mdp requested review from heifner and taokayan August 21, 2023 20:27
return static_cast<hook<index0_type, Allocator>&>(to_node(obj))._color;
}
static void set_removed_field(const value_type& obj, int val) {
static_cast<hook<index0_type, Allocator>&>(to_node(obj))._color = val;
}
Copy link
Member

Choose a reason for hiding this comment

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

I like this change.

// A concern could be that `1` is a special value meaning `nullptr`. However we could not
// get an offset of 4, since `sizeof(offset_node_base) == 16`, so we could not have
// difference between two different `node_ptr` be less than 16.
// --------------------------------------------------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

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

Did you run any performance comparisons with these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I didn't. Do you know of a good way to make a performance comparison? Maybe it would require to write a small benchmarking test.

Copy link
Member

Choose a reason for hiding this comment

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

You could write one I suppose, but you could also just do a replay comparison of some range of blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'll write a small test.

@greg7mdp greg7mdp merged commit b81a6dc into main Aug 29, 2023
@greg7mdp greg7mdp deleted the greg_reduce_memory branch August 29, 2023 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants