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

reduce memory allocation when create ReadOptions object #69

Closed
wants to merge 2 commits into from

Conversation

zhangjinpeng87
Copy link
Member

No description provided.

@@ -158,7 +158,7 @@ impl UnsafeSnap {

pub struct ReadOptions {
inner: *mut DBReadOptions,
upper_bound: Vec<u8>,
upper_bound: Option<Vec<u8>>,
Copy link

Choose a reason for hiding this comment

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

better use Option<Box<Vec<T>>>

Copy link
Member Author

Choose a reason for hiding this comment

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

For what reason?

Copy link

Choose a reason for hiding this comment

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

smaller size.

  • Option<Vec> -> 4 usize (a pointer, a length, a capacity, a tag)
  • Box<Vec<_>> -> 1 usize (a pointer)
  • Option<Box<Vec>> -> 1 usize (a pointer, since Box impls NonZero optimization)

Copy link

Choose a reason for hiding this comment

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

Option<Box<Vec<T>>> is used in rust implementation, called ThinVec<T>

Copy link
Member

Choose a reason for hiding this comment

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

Vec points a continuous memory on heap already, I think it's ridiculous to use Box again, which will allocate again.

Choose a reason for hiding this comment

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

does it really cause the memory problem?

Copy link

Choose a reason for hiding this comment

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

@siddontang no
@BusyJay this is the optimization for a normally-empty Vec. not ridiculous or something.

@BusyJay
Copy link
Member

BusyJay commented Jun 19, 2017

I don't think vec![] will allocate any memory on heap.

@andelf
Copy link

andelf commented Jun 19, 2017

struct crocksdb_readoptions_t {
   ReadOptions rep;
   Slice upper_bound; // stack variable to set pointer to in ReadOptions
};

ReadOptions is a large struct.

better use singleton pattern?

@BusyJay
Copy link
Member

BusyJay commented Jun 19, 2017

A better way to do this is add a new C ABI to support snapshot read (event support upper bound). In that way, crocksdb_readoptions_t will be allocated on stack.

@andelf
Copy link

andelf commented Jun 19, 2017

Wraps Options into Cow. export copy ctor.

@zhangjinpeng87
Copy link
Member Author

In most cases the upper_bound is not set, this pr just use None instead of vec![].

@siddontang
Copy link

CI failed

@BusyJay
Copy link
Member

BusyJay commented Jun 20, 2017

So why replacing vec![] with None?

@zhangjinpeng87
Copy link
Member Author

Close this pr since vec![] will not allocate any memory in heap.

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.

None yet

4 participants