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

Fix memory leak in groot and add trace logs #2971

Merged
merged 7 commits into from Jul 5, 2023

Conversation

siyuan0322
Copy link
Collaborator

@siyuan0322 siyuan0322 commented Jul 5, 2023

What do these changes do?

  1. Fix memory leak in groot
  2. Fix compiler warnings
  3. Bump up version of rocksdb and crossbeam-epoch
  4. Add trace logs

Related issue number

Fixes #2819

Committed-by: siyuanzhang.zsy from Dev container
Committed-by: siyuanzhang.zsy from Dev container
Committed-by: siyuanzhang.zsy from Dev container
Committed-by: siyuanzhang.zsy from Dev container
sighingnow
sighingnow previously approved these changes Jul 5, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #2971 (7646211) into main (731d5a3) will decrease coverage by 2.71%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2971      +/-   ##
==========================================
- Coverage   45.07%   42.37%   -2.71%     
==========================================
  Files          99       99              
  Lines       10660    10649      -11     
==========================================
- Hits         4805     4512     -293     
- Misses       5855     6137     +282     

see 15 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 731d5a3...7646211. Read the comment docs.

Committed-by: siyuanzhang.zsy from Dev container
Committed-by: siyuanzhang.zsy from Dev container
@@ -49,7 +49,7 @@ impl Drop for EnginePortsResponse {
fn drop(&mut self) {
unsafe {
if !self.errMsg.is_null() {
CString::from_raw(self.errMsg as *mut c_char);
drop(CString::from_raw(self.errMsg as *mut c_char));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can remove this unnecessary code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is for fixing the warnings of compiler.

unsafe {
Box::from_raw(ptr);
drop(Box::from_raw(ptr));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can drop this unnecessary code

Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly, if a point already been wrapped into a Rust struct, e.g. Box, or CString, they will be dropped automatically

Copy link
Collaborator

Choose a reason for hiding this comment

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

There would be a warning like

warning: unused return value of `std::boxed::Box::<T>::from_raw` that must be used
   --> /Users/runner/work/GraphScope/GraphScope/interactive_engine/executor/engine/pegasus/pegasus/src/communication/buffer.rs:121:13
    |
121 |             Box::from_raw(ptr.as_ptr());
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: call `drop(Box::from_raw(ptr))` if you intend to drop the `Box`
    = note: `#[warn(unused_must_use)]` on by default
help: use `let _ = ...` to ignore the resulting value
    |
121 |             let _ = Box::from_raw(ptr.as_ptr());
    |             +++++++

@@ -78,7 +78,7 @@ impl Drop for JnaResponse {
fn drop(&mut self) {
unsafe {
if !self.errMsg.is_null() {
CString::from_raw(self.errMsg as *mut c_char);
drop(CString::from_raw(self.errMsg as *mut c_char));
Copy link
Collaborator

Choose a reason for hiding this comment

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

drop() not necessary.

@siyuan0322 siyuan0322 merged commit ded982e into alibaba:main Jul 5, 2023
25 checks passed
@siyuan0322 siyuan0322 deleted the zsy/mem branch July 5, 2023 06:37
siyuan0322 added a commit that referenced this pull request Jul 5, 2023
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.

[BUG] The memory usage of Store in Groot will gradually increase
5 participants