Fix memory leak in table setters#220
Open
joaopaulosr95 wants to merge 1 commit into
Open
Conversation
PySAM_table_setter and PySAM_assign_from_dict convert the incoming dict to a SAM_table and pass it to the setter function, which copies the table into the module's data. The intermediate table was only destructed on the error path, leaking the full table on the successful path, thus causing services that build a model per request leak until OOM. Adds a peak-RSS regression test (POSIX only).
joaopaulosr95
force-pushed
the
fix/table-setter-memory-leak
branch
from
a7c0fa0 to
b4af536
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
PySAM_table_setterconverts the incoming Python dict to aSAM_tableand passes it to the generated*_tsetfunction, which copies the table into the module's data. The intermediate table is only destructed on the error path, so every successful dict assignment to a table variable leaks the full converted table.For
Pvwattsv8.SolarResource.solar_resource_datawith an 8760-row weather file this is ~1.2MB leaked per assignment. Long-running services that build a model per request (e.g. a REST API wrapping PVWatts) leak until OOM. The leak is unreachable from Python.PySAM_assign_from_dict(theModel.assign()path) has the same leak on both its success and error paths.Evidence
Repeat-loop RSS measurement (macOS arm64, Python 3.13, PySAM 7.1.0), assigning the same prebuilt dict to fresh
Pvwattsv8models, GC forced, live Python object counts flat:m.SolarResource.solar_resource_data = dm.assign({"SolarResource": {"solar_resource_data": d}})pvwatts.new()aloneThat the
*_tsetfunctions copy (rather than take ownership) is confirmed by the nested-table branch ofPySAM_dict_to_tableitself, which already destructs its intermediate right after a successfulSAM_table_set_table.Verification
ac_annualand all exported outputs bit-identical before/after the fix.tests/test_memory_leak.py(peak-RSS bound over 100 assignments, POSIX-only). It runs its measurement loop in apython -Isubprocess so the peak-RSS reading is immune to test ordering and to the repo-rootPySAM/dir shadowing the installed package. Fails on stock wheels, passes on patched builds.pytest tests/test_pysam_all.py):python:3.13container (manylinux2014 wheel): 15/15 functional tests pass stock and patched; leak test fails stock (+62.4MB), passes patched.nrel-pysam==7.1.1.post1) on both platforms; it is not specific to an OS or an older release.Additional notes
test_pkg.ymlinstalls the published wheel rather than building from source, so the new regression test will fail on the macOS jobs until a wheel containing this fix is published. It passes against any wheel built from a patched source tree.