ComputationalRadiationPhysics / redGrapes

Resource-based, Declarative task-Graphs for Parallel, Event-driven Scheduling :grapes:
https://redgrapes.rtfd.io
Mozilla Public License 2.0
20 stars 5 forks source link

fix memory leak #45

Closed psychocoderHPC closed 10 months ago

psychocoderHPC commented 10 months ago

With #44 we tried to fix the memory leak which can happen if more than one thread is allocating a new chunk. The PR #44 was not fixing the issue and introduced a double free issue.

@michaelsippel I have not tested the PR and wrote it blindly, we need to perform a runtime test before merging.

psychocoderHPC commented 10 months ago

@michaelsippel I applied your review comments and rebased against #47

psychocoderHPC commented 10 months ago

generally there is a confusion between 'chunks' & 'items' in the variable naming & comments. Otherwise I have a commit that refactors the Chunk such that it stores direct pointers instead of indices and we should build on that to implement the fix

I am fine if we refactor these parts in a follow up but as long as there is no PR fix that instantly fixes the current issue of memory leaks I would like to bring it in to have a version that is working correctly.

I am also fine if this PR is not going in, both ways are possible as long as the dev branch becomes usable again.

michaelsippel commented 10 months ago

For testing we should use small chunk sizes with many tasks since otherwise when everything fits into one chunk, the implementation will not be stressed

psychocoderHPC commented 10 months ago

Unfortunately, this PR creates a deadlock :-( during testing

michaelsippel commented 10 months ago

now already included in #48