fix double free and double alloc #1
Conversation
|
btw if you merge this the changes will be directly visible in your PR aginst the mainline. |
|
|
||
| release_chunk(chunk); | ||
| else | ||
| chunk->next_item.fetch_sub(1); |
There was a problem hiding this comment.
since we increase a 64bit pointer there should be no strong need to add this overflow guard here.
| { | ||
| *next_item = item; | ||
| chunk->last_item ++; | ||
| if( (uintptr_t)next_item == (uintptr_t)(chunk_end - 1u)) |
There was a problem hiding this comment.
This could be moved to the position before the item is copied to provide faster new memory to other threads.
michaelsippel
force-pushed
the
topic-chunkedlist
branch
2 times, most recently
from
6933ef3 to
8897117
Compare
Equally to ComputationalRadiationPhysics#45 this PR should solve the possible double free and double alloc.
psychocoderHPC
force-pushed
the
fix-doubleFreeAndAlloc
branch
from
9335d82 to
fb64cd8
Compare
|
@michaelsippel I rebased this PR against your latest changes and solved the conflicts. |
| @@ -424,7 +417,9 @@ struct ChunkedList | |||
| public: | |||
| ChunkedList( Allocator && alloc ) | |||
| : chunks( std::move(alloc), T_chunk_size * sizeof(Item) + sizeof(Chunk) ) | |||
| {} | |||
| { | |||
| chunks.allocate_item(); | |||
There was a problem hiding this comment.
I currently don't know how much the impact is, but this will decrease the speed of task-creation since every task will create four events , each containing a ChunkedList<EventPtr> to store the list of outgoing edges, but not all Events will actually have outgoing edges (this is also described in Issue ComputationalRadiationPhysics#42 . Maybe, to avoid this a possible condition for allocating a new chunk would be next_item == chunk_end ?
There was a problem hiding this comment.
next_item == chunk_end
I think if you use this more than one thread can create the first chunk. Thats the reason why I added it to the constructor because this was the only point where I was sure that there will be no data race.
michaelsippel
merged commit b22c4a9
into
michaelsippel:topic-chunkedlist
Equally to ComputationalRadiationPhysics#45 this PR should solve the possible double free and double alloc.