-
Notifications
You must be signed in to change notification settings - Fork 115
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
Ensure Java garbage collection #298
Conversation
Codecov Report
@@ Coverage Diff @@
## master #298 +/- ##
=======================================
Coverage 96.04% 96.04%
=======================================
Files 42 42
Lines 4677 4677
=======================================
Hits 4492 4492
Misses 185 185 Continue to review full report at Codecov.
|
9bfb6d2
to
fbe82fe
Compare
Following test was executed to test Java export to GDX
20 scenarios were exported from DB without any issues (see memory consumption on screenshot).
It has to be tested also uploading solutions (or any other methods called upon solve) to ensure they has no leaks. |
@OFR-IIASA reported (via Slack) that the changes here from April 15 ("Allow disabling JDBCBackend caching", currently commit # 77538ee, but will change if the branch is rebased) do not resolve the issue in real-world usage. The commits today add a new test, Experiment 1:
Other experiments:
Some thoughts at first glance:
Finally, a disclaimer that I have no experience in performance profiling, so I could be misinterpreting these numbers. |
b65c59d
to
4dcf013
Compare
The latest commit "Also write to GDX in test_reload_cycle" does what's on the label, per discussion this morning in Slack. I also adjusted @zikolach over to you for the advanced profiling. |
Some information was shared in Slack. As well, @OFR-IIASA reported:
This is encouraging—I guess we can continue to wait and see if the problem recurs or not. If not (after a week so), we can imagine that these changes (or others in the code @OFR-IIASA is running) collectively resolved it. |
267abe8
to
efcca04
Compare
@zikolach I'm seeing failures across CI platforms with similar messages like this. This test isn't related to the Python code changed in this PR, so I'm wondering why a Scenario would not be in a checked-in state immediately after it is solved. Might this have anything to do with the changes to ixmp.jar on this branch? |
Some recent issues I have been having using this branch. I have had this happen with setting a scenario to default, but also in the case below:
|
Here another example of a different error but with the same root cause
|
81ea232
to
02daad6
Compare
This turned out to be the case; the error (same as #298 (comment) and #298 (comment)) was resolved by the commit titled "Updated ixmp.jar built from [ixmp_source] b1b60ca…", currently 8aa8deb. |
Also, note that I just ran:
These all passed, in about 90 minutes. |
after pulling this branch and reinstalling ixmp, i restarted the script to receive the following error.
|
This was due to mismatched versions of ixmp and message_ix. |
- Use a 2D parameter in testing.add_random_model_data - Appease Stickler
6fadd3d
to
abdf7a5
Compare
- Adjust for iiasa/ixmp#298: the former tutorial syntax caused the Platform object to be garbage collected as soon as it was used, preventing use of the Scenario object. Assign it to an object reference to keep it alive until the end of the tutorial.
- Adjust for iiasa/ixmp#298: the former tutorial syntax caused the Platform object to be garbage collected as soon as it was used, preventing use of the Scenario object. Assign it to an object reference to keep it alive until the end of the tutorial.
- Adjust for iiasa/ixmp#298: the former tutorial syntax caused the Platform object to be garbage collected as soon as it was used, preventing use of the Scenario object. Assign it to an object reference to keep it alive until the end of the tutorial.
This PR responds to reports from @OFR-IIASA that JDBCBackend was exhausting memory after loading ~5 Scenarios of the MESSAGEix-GLOBIOM global model.
UPD: symptom was Java toGDX was throwing OutOfMemory exception due to the objects from heap could not be garbage collected (having referenced either from java or from python via JPype1).
The changes:
del_ts
, called byTimeSeries.__del__
.This signals the Backend to perform any clean-up when the TimeSeries (or Scenario) object is cleaned up.
CachingBackend.del_ts
empties the Python cache associated with the TimeSeries object that's being deleted. Addtest_cache_del_ts
to test this behaviour.JDBCBackend.jindex
to use a WeakKeyDictionaryTechnical explanation: Formerly this was a dict of (Python TimeSeries/Scenario object → Java TimeSeries/Scenario object, via JPype); but the keys referencing the TimeSeries objects prevented them from being garbage collected. WeakRefDictionary ensures that the dict key is not counted as a reference to the object.
Add
test_del_ts()
to test this.{TimeSeries,Scenario}.platform
are now proxy objects, with the same logic: an existing Scenario that references a Platform does not preventdel platform
. Addtest_weakref
to test this.test_jdbc.test_gc
and the pytest marktest_gc
are added (by @zikolach) to check that Java-side garbage collection under JDBCBackend functions as intended.May fix #227.
How to review
Confirm that a reasonable number of 'large' Scenarios can be loaded using this code.
PR checklist
Documentation added.N/A