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

Reducing memory leak related to accessing GRIB keys #320

Merged
merged 4 commits into from
Nov 16, 2022

Conversation

sandorkertesz
Copy link
Contributor

@sandorkertesz sandorkertesz commented Nov 15, 2022

Helps reducing the memory leak reported in #283.

The problem was related to the way cfgrib accessed ecCodes keys. It used

eccodes.codes_get_array()

exclusively. However, this seems to have caused a memory leak (inside ecCodes), especially when accessing string keys (e.g. "shortName"). By avoiding using eccodes.codes_get_array() for non-array keys the memory leak reduced significantly, although it is still present.

A comparison:

Using a GRIB file containing 420 messages, if we execute the following function 500 times:

def run():
    ds = xr.open_dataset(fname, engine="cfgrib", backend_kwargs={"indexpath": ""})
    total = len(ds)
    ds = None

the final RSS memory usage with the original code is 1334 MB, while with the fix it is only 96 MB.

@FussyDuck
Copy link

FussyDuck commented Nov 15, 2022

CLA assistant check
All committers have signed the CLA.

@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2022

Codecov Report

Base: 95.49% // Head: 95.40% // Decreases project coverage by -0.09% ⚠️

Coverage data is based on head (1804c8f) compared to base (a317a88).
Patch coverage: 77.77% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #320      +/-   ##
==========================================
- Coverage   95.49%   95.40%   -0.10%     
==========================================
  Files          26       26              
  Lines        2020     2022       +2     
  Branches      233      234       +1     
==========================================
  Hits         1929     1929              
- Misses         61       62       +1     
- Partials       30       31       +1     
Impacted Files Coverage Δ
cfgrib/messages.py 87.67% <77.77%> (-0.51%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

values = ["unsupported_key_type"]
return "unsupported_key_type"

if is_array and len(values) == 1:
Copy link
Member

Choose a reason for hiding this comment

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

It looks to me like this condition will never be met

Copy link
Member

Choose a reason for hiding this comment

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

Actually, it does seem to be met occasionally

@iainrussell iainrussell merged commit d70035f into ecmwf:master Nov 16, 2022
@iainrussell
Copy link
Member

Thanks @sandorkertesz, this fixes the majority of the leaking memory and should be a great help to users!

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.

4 participants