ComputationalRadiationPhysics / imresh

Shrink-Wrap Phase Reconstruction Algorithm
MIT License
3 stars 2 forks source link

We need free[dom] #27

Open mxmlnkn opened 8 years ago

mxmlnkn commented 8 years ago

The example imresh/examples/threadedExample.cpp suggests the following call to free the allocated data where justFree is just a wrapper to delete[]. This will be run in parallel and is bad (Segmentation fault).

// Now we can run the algorithm for testing purposes and free the data
// afterwards
imresh::io::addTask( file.first, file.second,
                     imresh::io::writeOutFuncs::justFree,
                     "free" /* gives an identifier for debugging */ ); 

Possible fixes:

The problem with letting the user do it are plentiful, because it breaks paralellism or is just too complex. My suggestion is to convert taskQueue.cu into a class e.g. called ShrinkWrapBatch so that adding tasks could be as easy as:

{
    ShrinkWrapBatch batch( true /* automatically free given data */ );
    while ( /* files in directory */ )
    {
        batch.add( readFile( filename ) );
    }
} // batch.~batch will be called

Note that no call to initialize is necessary anymore because it would be included in the constructor. Also no need to call deinit is necessary, because the outer scope will call the Destructor. Each data given is freed either by batch.add or batch.~batch

Ferruck commented 8 years ago

I already tried to implement a class-using version but this led to not-easy-to-understand CUDA errors. With an eye on the schedule I think this is a important task but a task for the future.

mxmlnkn commented 8 years ago

I think it would be a tad cooler, if it wasn't inside the write out functions, but if instead taskQueue.cu could create a [lambda]-wrapper which calls first the write out function and then delete[]. That way the user doesn't have to add it to their customized write out functions and writeOutPNG and writeOutHDF5 could be used for debugging output without having to worry about involuntary frees.

Ferruck commented 8 years ago

Having the free hardcoded into taskQueue.cu could lead to problems when the user doesn't want to free the pointer. But I don't know if that's an actual use-case.