[llvm] r249615 - Support: Stop using iplist in Recycler

[Mehdi Amini via llvm-commits]

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