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

Remove all possibly extraneous DeleteAthenaArray calls #560

Merged
merged 10 commits into from
Feb 19, 2024

Conversation

felker
Copy link
Contributor

@felker felker commented Jan 31, 2024

Closes #559.

@yanfeij can you play around with this branch and see if anything sticks out?

tomidakn
tomidakn previously approved these changes Jan 31, 2024
@felker
Copy link
Contributor Author

felker commented Feb 5, 2024

@changgoo what was the Jenkins failure? Can you add Princeton NetID kfelker to the tigris Jenkins job runner with view permissions?

@changgoo
Copy link
Contributor

changgoo commented Feb 5, 2024

/opt/rh/gcc-toolset-10/root/usr/bin/ld: /scratch/gpfs/changgoo/jenkins/workspace/tigris/athena-PR/tst/regression/obj/radiation.o: in function `NRRadiation::~NRRadiation()':
radiation.cpp:(.text+0x19): undefined reference to `RadIntegrator::~RadIntegrator()'
collect2: error: ld returned 1 exit status

@changgoo
Copy link
Contributor

changgoo commented Feb 5, 2024

I'm not sure how I can add you, but the error log has been sent to you (felker@anl.gov)

@felker
Copy link
Contributor Author

felker commented Feb 6, 2024

@yanfeij were you able to compare this branch vs. master for AMR memory usage in the problems you were concerned about?

@yanfeij
Copy link
Contributor

yanfeij commented Feb 6, 2024 via email

@yanfeij
Copy link
Contributor

yanfeij commented Feb 12, 2024

@felker So I did a check with amr and radiation. I created 888 mesh blocks and destroyed 708. Valgrind still reported that all heap blocks were freed. So I think this is fine to merge.

@felker
Copy link
Contributor Author

felker commented Feb 13, 2024

retest this please

@felker
Copy link
Contributor Author

felker commented Feb 13, 2024

@yanfeij looks like rad_source.py is failing

Terminating on cycle limit
time=6.2500000000000042e-02 cycle=50
tlim=2.0000000000000000e+00 nlim=50

Number of MeshBlocks = 64; 0  created, 0 destroyed during this simulation.

zone-cycles = 51200
cpu time used  = 4.3630600000000003e-01
zone-cycles/cpu_second = 1.1734883315837967e+05
error in case 1: tgas or Er 1.5 10.0
nr_radiation.rad_source test: prepare(), run(), analyze() finished

Results:
    nr_radiation.amr_linwave: passed; time elapsed: 63.2 s
    nr_radiation.rad_linearwave: passed; time elapsed: 458 s
    nr_radiation.rad_source: failed; time elapsed: 15.6 s

Summary: 2 out of 3 tests passed

@yanfeij
Copy link
Contributor

yanfeij commented Feb 14, 2024

@yanfeij looks like rad_linearwave.py is failing

Terminating on cycle limit
time=6.2500000000000042e-02 cycle=50
tlim=2.0000000000000000e+00 nlim=50

Number of MeshBlocks = 64; 0  created, 0 destroyed during this simulation.

zone-cycles = 51200
cpu time used  = 4.3630600000000003e-01
zone-cycles/cpu_second = 1.1734883315837967e+05
error in case 1: tgas or Er 1.5 10.0
nr_radiation.rad_source test: prepare(), run(), analyze() finished

Results:
    nr_radiation.amr_linwave: passed; time elapsed: 63.2 s
    nr_radiation.rad_linearwave: passed; time elapsed: 458 s
    nr_radiation.rad_source: failed; time elapsed: 15.6 s

Summary: 2 out of 3 tests passed

@felker I added a problem generator with amr to check memory leak, which was used for the regression test instead of the original one. I have fixed this. Can you try again?

@changgoo
Copy link
Contributor

retest this on stellar

@felker felker requested a review from c-white as a code owner February 14, 2024 02:19
@felker
Copy link
Contributor Author

felker commented Feb 14, 2024

@yanfeij should I edit the new test in some way?

Terminating on time limit
time=2.0000000000000000e+00 cycle=1601
tlim=2.0000000000000000e+00 nlim=100000

zone-cycles = 1639424
cpu time used  = 8.8863350000000008e+00
zone-cycles/cpu_second = 1.8448820576761960e+05
nr_radiation.rad_source test: prepare(), run(), analyze() finished

Results:
    nr_radiation.amr_linwave: passed; time elapsed: 57.6 s
    nr_radiation.amr_source: failed; time elapsed: 15.8 s
    nr_radiation.rad_linearwave: passed; time elapsed: 435 s
    nr_radiation.rad_source: passed; time elapsed: 41.4 s

Summary: 3 out of 4 tests passed

@yanfeij
Copy link
Contributor

yanfeij commented Feb 14, 2024

@yanfeij should I edit the new test in some way?

Terminating on time limit
time=2.0000000000000000e+00 cycle=1601
tlim=2.0000000000000000e+00 nlim=100000

zone-cycles = 1639424
cpu time used  = 8.8863350000000008e+00
zone-cycles/cpu_second = 1.8448820576761960e+05
nr_radiation.rad_source test: prepare(), run(), analyze() finished

Results:
    nr_radiation.amr_linwave: passed; time elapsed: 57.6 s
    nr_radiation.amr_source: failed; time elapsed: 15.8 s
    nr_radiation.rad_linearwave: passed; time elapsed: 435 s
    nr_radiation.rad_source: passed; time elapsed: 41.4 s

Summary: 3 out of 4 tests passed

Oh, I was not thinking adding it to the regression test. I used valgrind to check no memory leak after all the meshblocks are created and deleted. I am not sure how to do it automatically. I added the problem generator and input file so that people can do it manually if needed.

Use Valgrind with athinput.thermal_relaxation_amr
to check that code is memory leak-free
@felker felker merged commit 525bed6 into master Feb 19, 2024
1 check passed
@felker felker deleted the hotfix/delete_athenaarray branch February 19, 2024 16:47
@felker felker added the bug Broken functionality or unexpected result label Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Broken functionality or unexpected result
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove extraneous DeleteAthenaArray() calls
4 participants