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

Fixing memory leaks #34

Draft
wants to merge 6 commits into
base: dev
Choose a base branch
from
Draft

Fixing memory leaks #34

wants to merge 6 commits into from

Conversation

FJShen
Copy link

@FJShen FJShen commented Mar 26, 2022

The old PR #31 is closed. This PR is tracking a dedicated branch "FJShen:leakfix" .

@JRPan JRPan requested review from a team and Connie120 and removed request for a team April 11, 2022 17:51
Copy link

@tgrogers tgrogers left a comment

Choose a reason for hiding this comment

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

Another good change that died on the vine @FJShen - what would it take to move this from "draft" to "done"?

@FJShen
Copy link
Author

FJShen commented Feb 15, 2025

I think that will involve a lot of work. To merge this PR itself is just about pulling in the latest changes from the dev branch (and resolving any conflicts), but there's no guarantee that this PR solves the memory leak problem. Primary reason is that, there are so many raw "new" allocations everywhere in the code, including cases where the program allocates a vector of elements, but deallocate them individually here and there (if I recall correctly). To rectify these issues involves some vast restructuring. Valgrind does not offer much help.

On the other hand I did not notice significant memory leak at a high level, in the form of continuous memory footprint growth. Reports of memory leak from the community were quite sparse.

@tgrogers
Copy link

OK, thanks, Fangjia!

I don't think we have a serious leak problem, but we do have a serious orthogonal over-allocation problem with large traces.
Should we just kill this PR then? Or do you feel there is valuable work here that is worth getting working at some point?

2 other points:

  1. Vaggrind should always help if you use it correctly
  2. I am not sure who "you" is in your comment. You can't blame me for all the code in the simulator :) Try not to personify code, people tend to get upset.

@FJShen
Copy link
Author

FJShen commented Feb 16, 2025

Sorry for the misunderstanding. I don't blame you @tgrogers I've modified the comment to de-personify the statement.

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.

2 participants