[PATCH] [PATCH/RFC] Use a BumpAllocator for DIEs

[David Blaikie]

On Tue, Mar 10, 2015 at 12:14 PM, Frederic Riss <friss at apple.com> wrote:

> ================
> Comment at: include/llvm/CodeGen/DIE.h:135
> @@ -110,3 +134,3 @@
>
> -class DIE {
> +class DIE : public ilist_node<DIE> {
>  protected:
> ----------------
> dblaikie wrote:
> > This might be able to be private inheritance? (just handle the DIEs by
> unique_ptr and then pass them into addChild)
> >
> > Hmm, I suppose it'd need to friend the ilist instantiation & maybe other
> things, for this to be private inheritance?
> Could try it, though I don't think I've seen that pattern anywhere else.
>

Fair enough *shrug* up to you, just a thought.


>
> ================
> Comment at: include/llvm/CodeGen/DIE.h:157
> @@ -132,3 +156,3 @@
>    // be more convoluted than beneficial.
> -  std::vector<std::unique_ptr<DIE>> Children;
> +  ilist<DIE> Children;
>
> ----------------
> dblaikie wrote:
> > I've a bit of an aversion to intrusive lists - so I wonder if you'd be
> willing to revisit the issues in the comment above (including the issue
> about why list<DIE> was insufficient - picking up from where I left off to
> see if we could reasonably rely on post-insertion pointer validity without
> too much convolution?).
> >
> > Actually why move to ilist? These objects will never have their dtors
> run, right? So you could just switch from vector<unique_ptr<DIE>> to
> vector<DIE*> - everything's non-owning now, since their memory is managed
> by the BumpPtrAllocator entirely?
> If we use list<DIE>, we'd need to use a std custom allocator that wraps
> the BumpPtrAllocator which would complicate things quite a bit. I could try
> it though.


Right right.


> The whole point of the ilist here is that allocating a DIE allocates
> everything that's needed for the list bookkeeping.

Re: vector<DIE*>, you'd leak the vector storage if you do not call the
> destructor on every DIE.
>

Ah, right.


>
> ================
> Comment at: include/llvm/CodeGen/DIE.h:220
> @@ -184,3 +219,3 @@
>      Child->Parent = this;
>      Children.push_back(std::move(Child));
>    }
> ----------------
> dblaikie wrote:
> > If we're going to go with ilist, could we improve/overload ilist's
> push_back to take unique_ptr for clarity?
> I can try that.
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:67
> @@ -66,2 +66,3 @@
>           UnitTag == dwarf::DW_TAG_type_unit);
> +  UnitDie = new (DIEValueAllocator) DIE(UnitTag);
>    DIEIntegerOne = new (DIEValueAllocator) DIEInteger(1);
> ----------------
> dblaikie wrote:
> > Doesn't seem like it'd be too important to allocate the unit die with
> the DIEValueAllocator, does it?
> >
> > I guess if you don't, then it can end up in the list of DIEs to
> deallocate, and ends up double-deleted? Again, this would be the sort of
> subtlety it might be best to avoid.
> It's not important from a performance standpoint, but if I find the logic
> easier to follow if the rule is "allocate everything DIE related on the
> BumpPtrAllocator". If we allow this on to be a an exception to this rule,
> we must then handle its destruction specifically (as you point out,
> removing it from the DIEsToDelete list, but also making sure it drops its
> children and does not delete them).
>

Yeah, I'm really a bit concerned about the subtlety of the DIE type and how
easily it could be misused as it stands.


>
> ================
> Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.h:116
> @@ -113,1 +115,3 @@
> +  // called so that the SmallVector's memory isn't leaked.
> +  std::vector<DIE*> DIEsToDelete;
>
> ----------------
> dblaikie wrote:
> > This seems a bit subtle - having the SmallVector's dtors not be run
> unless they happen to go over the limit & reallocate. Could we use the
> existing BumpPtrAllocator as the allocator to the SmallVectors? Or
> something similar. (perhaps we discussed this, it's been a while)
> SmallVector don't accept configurable allocators AFAICS. I tried going
> with a std::vector with a custom allocator that wrapped a mix of recycliing
> + BumpPtrAllocator, but it didn't perform well.
>
> I agree it's subtle, but it's the best tradeoff I could find.
>
> ================
> Comment at: unittests/CodeGen/DIEHashTest.cpp:626
> @@ -625,3 +625,3 @@
>    DIEEntry PITy(*PITyDIE);
> -  auto PI = make_unique<DIE>(dwarf::DW_TAG_member);
> +  DIE *PI = new DIE(dwarf::DW_TAG_member);
>    DIEString PIStr(&One, "PI");
> ----------------
> dblaikie wrote:
> > Again, bit subtle - in the real use case this is non-owning because the
> parent's dtor is never run, etc, etc. But in the test case it is owning
> because the parent's dtor is run?
> >
> > Hmm, that raises another question - once a DIE is added to the "DIEs to
> delete" list, what stops it from deleting its children DIEs? (since it's a
> member std::ilist in there) possibly leading to excess deletes
> (inefficiencies you're trying to remove) or double/triple/multiple deletes
> if children are in the "DIEs to delete" list too?
> This approach basically gives you the choice of ownership. If you wanna
> use something like a BumpPtrAllocator, then you use the DIEsToDelete trick
> to avoid leaking memory.
>
> If you wanna use a more 'traditional' memory management scheme, it's also
> possible like in this test.


I'm not sure this is a feature - it seems more like complexity that would
easily lead to mis-use...


> The DIEs are still owned by their parents through the ilist of children,
> which means calling the root destructor will destroy everything.
>
> This is handled in the BumpPtrAllocator by preventing the DIEs from the
> DIEsToDelete array to delete their children (look for
> clearAndLeakNodesUnsafely() in ~DwarfUnit). Yeah, subtle again...
>

Right.

Maybe it'd be best to move to a manually managed linked list, so it's clear
that this is all non-owning? Or at least have ~DIE's dtor explicitly
release ownership of the elements of the ilist (but even this I find to be
questionable - if these things aren't owned (& they're not - because they
might be in the DIESToDelete list) then we shouldn't put them in a
container that has ownership... )

- David
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150310/ab06826f/attachment.html>