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.

Sign up for GitHub

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

Jump to bottom

BUG: memleak part 4 #825

Merged
merged 2 commits into from
Oct 5, 2022
Merged

BUG: memleak part 4 #825

merged 2 commits into from
Oct 5, 2022

Conversation

shanedsnyder
Copy link
Contributor

@shanedsnyder shanedsnyder commented Oct 3, 2022 •
edited
Loading

  • dup() returned record name and destroy the name record hash table and all associated memory in functions darshan_log_get_name_records() and darshan_log_get_filtered_name_records()
  • free allocated array of record_id,record_name tuples passed back from above functions in CFFI backend
  • free individual record names passed back frome above functions in CFFI backend

Fixes #824

@shanedsnyder
fix memory name record mem leaks in darshan-util
c7f690d 
* dup() returned record name and destroy the name record hash
  table and all associated memory in functions
  darshan_log_get_name_records and
  darshan_log_get_filtered_name_records
* free allocated array of record_id,record_name tuples passed
  back from above functions in CFFI backend
* free individual record names passed back frome above functions
  in CFFI backend
@shanedsnyder
Copy link
Contributor Author

Memory profile looks much flatter now for reproducer in #824.
image

Although, you do see some slight increase over time, looks like basically a couple of MiB.

Extending the iterations:
image

Not sure what's going on as it seems to periodically reset, then rise again. Is it some sort of artifact of garbage collection happening in CFFI or something like that?

@tylerjereddy tylerjereddy added the bug Something isn't working label Oct 3, 2022
tylerjereddy
tylerjereddy reviewed Oct 3, 2022
View reviewed changes
Copy link
Collaborator

@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

The Python side looks good to me at least. I also confirmed a fix to the memory leak with ior_hdf5_example.darshan and the reproducer in the matching issue, after accounting for likely garbage collection oscillations:

image

@tylerjereddy tylerjereddy mentioned this pull request Oct 3, 2022
Open
carns
carns requested changes Oct 5, 2022
View reviewed changes
Copy link
Contributor

@carns carns left a comment

Choose a reason for hiding this comment

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

Looks good, I just have a minor suggestion to add comments to help remember what's going on here in the future. I think there are two instances to put it on. Otherwise looks good to me.

carns
carns approved these changes Oct 5, 2022
View reviewed changes
Copy link
Contributor

@carns carns left a comment

Choose a reason for hiding this comment

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

lgtm

@shanedsnyder shanedsnyder merged commit 17997fe into main Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tylerjereddy tylerjereddy tylerjereddy left review comments

@carns carns carns approved these changes

Assignees
No one assigned
Labels
bug Something isn't working pydarshan
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Another memory leak in PyDarshan
3 participants
@shanedsnyder @carns @tylerjereddy