[llvm] r249615 - Support: Stop using iplist in Recycler
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 7 14:34:05 PDT 2015
As you point out, I didn't introduce any UB here (although I may have
made it more obvious).
Is it actually problematic? Given that `FreeList` is POD -- and so
in-place constructors/destructors are no-ops -- I don't see how this
pattern is fundamentally different from using an
`AlignedCharArrayUnion` (it certainly smells funny to me, but so does
ACAU...). Does our UB-sanitizer have a check that fires? Since the
storage is reused sequentially (it will never alias) I don't think it
will have problems in practice. (The only thing that scares me is if
the allocation is smaller than a pointer.)
Anyway, no, I'm not planning to look at this. If someone does decide
to dig in, check out ArrayRecycler as well, since it's doing the same
thing.
> On 2015-Oct-07, at 14:07, Mehdi Amini <mehdi.amini at apple.com> wrote:
>
> Hi Duncan,
>
> It seems to me that after your patches, the Recycler is still involving UB right? Are you planning to come back to this when you’ll be done with the iplist?
>
> —
> Mehdi
>
>
>> On Oct 7, 2015, at 1:49 PM, Duncan P. N. Exon Smith via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>
>> Author: dexonsmith
>> Date: Wed Oct 7 15:49:09 2015
>> New Revision: 249615
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=249615&view=rev
>> Log:
>> Support: Stop using iplist in Recycler
>>
>> Recycler just needs a singly-linked list, and it takes less (and
>> simpler) code to hand-roll one of those than to build up the equivalent
>> `iplist_traits`. In theory, this should speed things up a bit too, but
>> this is really just a drive-by cleanup so I haven't measured.
>>
>> Modified:
>> llvm/trunk/include/llvm/Support/Recycler.h
>>
>> Modified: llvm/trunk/include/llvm/Support/Recycler.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Recycler.h?rev=249615&r1=249614&r2=249615&view=diff
>> ==============================================================================
>> --- llvm/trunk/include/llvm/Support/Recycler.h (original)
>> +++ llvm/trunk/include/llvm/Support/Recycler.h Wed Oct 7 15:49:09 2015
>> @@ -28,53 +28,36 @@ namespace llvm {
>> ///
>> void PrintRecyclerStats(size_t Size, size_t Align, size_t FreeListSize);
>>
>> -/// RecyclerStruct - Implementation detail for Recycler. This is a
>> -/// class that the recycler imposes on free'd memory to carve out
>> -/// next/prev pointers.
>> -struct RecyclerStruct {
>> - RecyclerStruct *Prev, *Next;
>> -};
>> -
>> -template<>
>> -struct ilist_traits<RecyclerStruct> :
>> - public ilist_default_traits<RecyclerStruct> {
>> - static RecyclerStruct *getPrev(const RecyclerStruct *t) { return t->Prev; }
>> - static RecyclerStruct *getNext(const RecyclerStruct *t) { return t->Next; }
>> - static void setPrev(RecyclerStruct *t, RecyclerStruct *p) { t->Prev = p; }
>> - static void setNext(RecyclerStruct *t, RecyclerStruct *n) { t->Next = n; }
>> -
>> - mutable RecyclerStruct Sentinel;
>> - RecyclerStruct *createSentinel() const {
>> - return &Sentinel;
>> - }
>> - static void destroySentinel(RecyclerStruct *) {}
>> -
>> - RecyclerStruct *provideInitialHead() const { return createSentinel(); }
>> - RecyclerStruct *ensureHead(RecyclerStruct*) const { return createSentinel(); }
>> - static void noteHead(RecyclerStruct*, RecyclerStruct*) {}
>> -
>> - static void deleteNode(RecyclerStruct *) {
>> - llvm_unreachable("Recycler's ilist_traits shouldn't see a deleteNode call!");
>> - }
>> -};
>> -
>> /// Recycler - This class manages a linked-list of deallocated nodes
>> /// and facilitates reusing deallocated memory in place of allocating
>> /// new memory.
>> ///
>> template<class T, size_t Size = sizeof(T), size_t Align = AlignOf<T>::Alignment>
>> class Recycler {
>> - /// FreeList - Doubly-linked list of nodes that have deleted contents and
>> - /// are not in active use.
>> - ///
>> - iplist<RecyclerStruct> FreeList;
>> + struct FreeNode {
>> + FreeNode *Next;
>> + };
>
>
>
>
>> +
>> + /// List of nodes that have deleted contents and are not in active use.
>> + FreeNode *FreeList = nullptr;
>> +
>> + FreeNode *pop_val() {
>> + auto *Val = FreeList;
>> + FreeList = FreeList->Next;
>> + return Val;
>> + }
>> +
>> + void push(FreeNode *N) {
>> + N->Next = FreeList;
>> + FreeList = N;
>> + }
>>
>> public:
>> ~Recycler() {
>> // If this fails, either the callee has lost track of some allocation,
>> // or the callee isn't tracking allocations and should just call
>> // clear() before deleting the Recycler.
>> - assert(FreeList.empty() && "Non-empty recycler deleted!");
>> + assert(!FreeList && "Non-empty recycler deleted!");
>> }
>>
>> /// clear - Release all the tracked allocations to the allocator. The
>> @@ -82,8 +65,8 @@ public:
>> /// deleted; calling clear is one way to ensure this.
>> template<class AllocatorType>
>> void clear(AllocatorType &Allocator) {
>> - while (!FreeList.empty()) {
>> - T *t = reinterpret_cast<T *>(FreeList.remove(FreeList.begin()));
>> + while (FreeList) {
>> + T *t = reinterpret_cast<T *>(pop_val());
>> Allocator.Deallocate(t);
>> }
>> }
>> @@ -93,9 +76,7 @@ public:
>> ///
>> /// There is no need to traverse the free list, pulling all the objects into
>> /// cache.
>> - void clear(BumpPtrAllocator&) {
>> - FreeList.clearAndLeakNodesUnsafely();
>> - }
>> + void clear(BumpPtrAllocator &) { FreeList = nullptr; }
>>
>> template<class SubClass, class AllocatorType>
>> SubClass *Allocate(AllocatorType &Allocator) {
>> @@ -103,9 +84,8 @@ public:
>> "Recycler allocation alignment is less than object align!");
>> static_assert(sizeof(SubClass) <= Size,
>> "Recycler allocation size is less than object size!");
>> - return !FreeList.empty() ?
>> - reinterpret_cast<SubClass *>(FreeList.remove(FreeList.begin())) :
>> - static_cast<SubClass *>(Allocator.Allocate(Size, Align));
>> + return FreeList ? reinterpret_cast<SubClass *>(pop_val())
>> + : static_cast<SubClass *>(Allocator.Allocate(Size, Align));
>> }
>>
>> template<class AllocatorType>
>> @@ -115,14 +95,20 @@ public:
>>
>> template<class SubClass, class AllocatorType>
>> void Deallocate(AllocatorType & /*Allocator*/, SubClass* Element) {
>> - FreeList.push_front(reinterpret_cast<RecyclerStruct *>(Element));
>> + push(reinterpret_cast<FreeNode *>(Element));
>> }
>>
>> - void PrintStats() {
>> - PrintRecyclerStats(Size, Align, FreeList.size());
>> - }
>> + void PrintStats();
>> };
>>
>> +template <class T, size_t Size, size_t Align>
>> +void Recycler<T, Size, Align>::PrintStats() {
>> + size_t S = 0;
>> + for (auto *I = FreeList; I; I = I->Next)
>> + ++S;
>> + PrintRecyclerStats(Size, Align, S);
>> +}
>> +
>> }
>>
>> #endif
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
More information about the llvm-commits
mailing list