BUG: Fix memory leaks by moving progress from the heap to the stack
#11
Conversation
Declaring those two local `ProgressReporter` objects on the stack (instead of on the heap) may benefit run-time performance. But moreover, it fixes two memory leaks (missing `delete progress`).
Declaring `ProgressReporter` on the stack (instead of on the heap) may benefit run-time performance. But moreover, it fixes a memory leak. - Corresponds with pull request richardbeare/parabolicMorphology#11 "BUG: Fix memory leaks by moving `progress` from the heap to the stack"
Declaring `ProgressReporter` on the stack (instead of on the heap) may benefit run-time performance. But moreover, it fixes a memory leak. - Corresponds with pull request richardbeare/parabolicMorphology#11 "BUG: Fix memory leaks by moving `progress` from the heap to the stack"
|
|
Minimally - I'm hoping to look at your mods when I'm back from leave.
…On Thu, Jul 4, 2024 at 8:02 PM Niels Dekker ***@***.***> wrote:
- @richardbeare <https://github.com/richardbeare> Are you still
maintaining this library? I think I have a follow-up already to this pull
request! Equivalent to SuperElastix/elastix#1184
<SuperElastix/elastix#1184>
—
Reply to this email directly, view it on GitHub
<#11 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAF6RIIYTMXON36XZ2T74XTZKUMU3AVCNFSM6AAAAABKHGZD46VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMBYGU4TINZVGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Thanks, @richardbeare In the mean time, I'll try to already get my proposed ParabolicMorphology adjustments into https://github.com/SuperElastix/elastix and https://github.com/InsightSoftwareConsortium/ITKParabolicMorphology, hoping that they will eventually find their way into your repo! |
|
The remote module in ITK references InsightSoftwareConsortium/ITKParabolicMorphology, rather than my repo. I'll have a look at what you've proposed. The ITK migration guide suggests we should be moving to ProgressTransformer. Perhaps this is the time to make the move. The changes in other PRs look OK. |
|
Thanks @richardbeare Of course, I can also submit the other PRs to your richardbeare/parabolicMorphology repo (rather than just to InsightSoftwareConsortium/ITKParabolicMorphology and elastix). What would you suggest? |
Thanks @richardbeare Honestly I didn't know about
Still not sure if it applies to Anyway, I would propose to just fix those two obvious memory leaks of local |
|
I've confirmed the leak existed and is fixed by this modification. (Had hoped that the use of auto would invoke the smart pointer side of things and handle the problem). |
Thank you @richardbeare, for confirming the memory leak, as well as the fix!
Well, I think it's OK already. The corresponding PR that I submitted to the InsightConsortium is already on top of the head of Insight's main branch: Can you possibly merge the pull request (Insight PR #45) at the Insight repo? |
|
I don't have control of the InsightConsortium fork - I guess they'll get to it soon. I'll sync my fork with theirs once the PR is merged. |
Thanks @richardbeare. I don't mind asking the maintainers of the InsightConsortium fork, but are you sure you don't have permission to merge? As far as I can see, you did merge the very first PR to the InsightConsortium fork! Back in 2015! (FYI, I regularly contribute to ITK, so I'm familiar with the other maintainers 😃. We also collaborate on elastix/ITKElastix.) |
|
@thewtex I'm very out of date with the PR process - are you able to look into these updates? |
|
@richardbeare I went ahead and merged them. If the PR's look good to you, feel free to just hit the green PR merge button. When we want to push new Python packages, we currently have to manually bump the version -- done here: InsightSoftwareConsortium#47 |
Declaring those two local
ProgressReporterobjects on the stack (instead of on the heap) may benefit run-time performance. But moreover, it fixes two memory leaks (missingdelete progress).