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

Replace MEM_DREAD_CTRL pool with MEMPROXY class pool #1924

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yadij
Copy link
Contributor

@yadij yadij commented Oct 28, 2024

Replacing memory allocate and destruct with C++ new/delete
and in-class initialization.

 ... to a MEMPROXY class pool.

Replacing memory allocate and destruct with C++ new/delete
and in-class initialization.
@rousskov rousskov changed the title Maintenance: Upgrade MEM_DREAD_CTRL memory pool Replace MEM_DREAD_CTRL pool with MEMPROXY class pool Oct 28, 2024
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

I adjusted PR title/description to match recent commit ed1a732.


Before advancing this new PR (including fixing build tests) and requesting my re-review, please cooperate to merge PRs awaiting your review for a long time, including these three:

Comment on lines +40 to +41
DRCB *handler = {};
void *client_data = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

By default, please use nullptr to initialize raw pointers.

Suggested change
DRCB *handler = {};
void *client_data = {};
DRCB *handler = nullptr;
void *client_data = nullptr;

Alternatively, do not initialize data members that are supposed to be set by the class constructor. This approach allows the compiler to warn us if we forget to copy constructor parameter to a class data member.

ctrl_dat->end_of_file = 0;
ctrl_dat->handler = handler;
ctrl_dat->client_data = cbdataReference(client_data);
const auto ctrl_dat = new dread_ctrl(fd, offset, req_len, buf, handler, cbdataReference(client_data));
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend moving cbdataReference() call inside dread_ctrl constructor and placing the matching cbdataReferenceDone() call inside its destructor. Doing so will lower several red flags:

  • Official diskHandleRead() code sometimes does not call cbdataReferenceDone() when deleting ctrl_dat object.
  • PR code prohibits dread_ctrl object moving or copying, but there is no obvious reason to do that (now) because the class effectively remains a POD.

I do not insist on this change despite recommending it.

@rousskov rousskov added the S-waiting-for-author author action is expected (and usually required) label Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-for-author author action is expected (and usually required)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants