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

Fix memory leak. #310

Closed

Conversation

WenzDaniel
Copy link
Collaborator

What is the problem / what does the code in this PR do
This PR is a different solution for the problem described in #309
Can you briefly describe how it works?
Inspired by the solution in #309. I moved the function specific_output into a dictionary outside of the PeakSplitter class which solved the problem
Can you give a minimal working example (or illustrate with a figure)?
run python straxer 170206_1355 --target peaklets --context xenon1t_dali --build_lowlevel --profile_ram

Here are some example outputs:

Begining:

Got 1547 items. Now 1.1 sec / 0.0% into the run. Using 514.9 MB RAM.
Got 1901 items. Now 2.0 sec / 0.1% into the run. Using 676.9 MB RAM. ETA 2602.18 sec.
Got 1654 items. Now 3.2 sec / 0.1% into the run. Using 731.0 MB RAM. ETA 3094.93 sec.
Got 1666 items. Now 4.2 sec / 0.1% into the run. Using 722.3 MB RAM. ETA 3326.21 sec.
Got 1780 items. Now 5.6 sec / 0.2% into the run. Using 792.1 MB RAM. ETA 3514.53 sec.

20 % into the run:

Got 1666 items. Now 807.6 sec / 22.4% into the run. Using 1169.3 MB RAM. ETA 3263.64 sec.
Got 1652 items. Now 808.7 sec / 22.4% into the run. Using 1175.7 MB RAM. ETA 3262.02 sec.
Got 1812 items. Now 809.7 sec / 22.5% into the run. Using 1129.8 MB RAM. ETA 3261.01 sec.
Got 2186 items. Now 810.9 sec / 22.5% into the run. Using 1251.8 MB RAM. ETA 3259.96 sec.

@JoranAngevaare
Copy link
Contributor

JoranAngevaare commented Aug 29, 2020

To be frank, knowing how much headache #309 caused I'm not sure if we should want to go down this road, unless it's truly significantly better but it doesn't make the code more resilient nor more readable. I would rather duplicate ~20 lines than having to deal with this issue again.

@WenzDaniel
Copy link
Collaborator Author

WenzDaniel commented Aug 29, 2020

I think this time we do not share the same point of view. I do not see why the issue should show up again, since we solved it and understood why it was caused in the first place. I am also not sure which solution is in the end more readable. It is kind of the same is not it? We defined externally two operations which gets called inside the PeakSplitter depending on the input, idk.
Personally, I am afraid that those copy-pasted lines could make us in the future some trouble in case people only modify one of the two.
Since you spend an entire day to track down this issue, I think it should be your call. However, I think the least thing we should do is a short warning in the doc strings of the two functions saying something like "Hey, pal when you modify me think about if you have to modify the other guy as well."

@WenzDaniel WenzDaniel closed this Aug 29, 2020
@WenzDaniel WenzDaniel deleted the patch_peak_splitting branch September 2, 2020 23:23
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.

2 participants