ChunkedList: store pointers to next/last elements instead of indices … #48
Conversation
|
Here I'm still getting warnings which I dont understand |
michaelsippel
force-pushed
the
topic-chunkedlist
branch
from
a0d1479
to
e2fd434
Compare
Could it be a hint that some data are unaligned? |
|
@michaelsippel Is this PR a replacement for #45? |
There was a problem hiding this comment.
IMO we still have the double free issue I tried to solve with #45 and we have the problem that more than one thread can append a new chunk.
redGrapes/util/chunked_list.hpp
Outdated
| #include <limits> | ||
| #include <memory> | ||
| #include <optional> | ||
| #include <redGrapes/util/trace.hpp> | ||
| #include <redGrapes/memory/allocator.hpp> | ||
| #include <redGrapes/memory/bump_allocator.hpp> | ||
| //#include <redGrapes/memory/bump_allocator.hpp> |
There was a problem hiding this comment.
?? why is this commented out?
There was a problem hiding this comment.
commented because its not needed. Allocator is a template parameter. will be removed
redGrapes/util/chunked_list.hpp
Outdated
| } | ||
|
|
||
| ~Chunk() | ||
| { | ||
| for( unsigned i = 0; i < last_idx; ++i ) | ||
| items()[i].~Item(); | ||
| for( Item * item = first_item; item != last_item; item++ ) |
There was a problem hiding this comment.
last_item is pointing by default to the element before the first item. If we never allocate memory but create a chunked_list does it mean we loop over non existing memory?
There was a problem hiding this comment.
maybe for( Item * item = first_item; item <= last_item ; item++ )?
There was a problem hiding this comment.
True, currently this bug doesnt manifest because a chunk is only allocated if at least one element exists , but when we switch to the algorithm where the thread with the last slot allocates the new chunk, it can happen that a chunk remains empty and thus creating an endless loop.
redGrapes/util/chunked_list.hpp
Outdated
| typename memory::AtomicList< Chunk, Allocator >::MutBackwardIterator chunk; | ||
|
|
||
| /* this pointer packs the address of the current element | ||
| * and the `has_element` bit in its MSB. |
There was a problem hiding this comment.
MSB = most significant bit
There was a problem hiding this comment.
please add this to the first usage
| @@ -441,7 +438,7 @@ struct ChunkedList | |||
| void release_chunk( typename memory::AtomicList< Chunk, Allocator >::MutBackwardIterator chunk ) | |||
| { | |||
| if( chunk->item_count.fetch_sub(1) == 0 ) | |||
| chunks.erase( chunk ); | |||
| chunks.erase( chunk ); | |||
There was a problem hiding this comment.
If a thread is entering this line and at the same moment another thread is pushing an item and is increasing the counter and sees that no item is free and then releases the memory again we will have a double free.
There was a problem hiding this comment.
in this version we increment item_count only after we are safe to have gotten a slot. Only if the increment of item_count happens first and then decrements this is relevant
redGrapes/util/chunked_list.hpp
Outdated
| } | ||
| } | ||
|
|
||
| release_chunk(chunk); | ||
| } | ||
|
|
||
| auto prev_chunk = chunks.allocate_item(); |
There was a problem hiding this comment.
How do we prevent that more than one thread is creating new memory?
|
Yes you are right, this PR does not include the fix for the double allocation / memory leak yet , #45 is still neccesary, either one needs to be rebased |
|
I opened a PR against your PR to solve the double free and alloc issue: michaelsippel#1 |
michaelsippel
force-pushed
the
topic-chunkedlist
branch
from
e2fd434
to
6933ef3
Compare
…to save overhead of address calculation and allow more efficient packing of ItemAccess * this also reintroduces the chunk-size as template parameter instead of storing it as member * initialize AtomicList with `chunk_capacity`, not allocation size * AtomicList: construct Item/ItemControlBlock from `memory::Block` * ChunkedList: construct Chunk from `memory::Block` * reorder ChunkedList::Item struct for more efficient alignment * encode `has_item` of ItemAccess into one 8-byte pointer using bitmagic
michaelsippel
force-pushed
the
topic-chunkedlist
branch
from
6933ef3
to
8897117
Compare
Equally to ComputationalRadiationPhysics#45 this PR should solve the possible double free and double alloc.
fix double free and double alloc
psychocoderHPC
merged commit 794431a
into
ComputationalRadiationPhysics:dev
…to save overhead of address calculation and allow more efficient packing of ItemAccess
chunk_capacity, not allocation sizememory::Blockmemory::Blockhas_itemof ItemAccess into one 8-byte pointer using bitmagic