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

David Blaikie dblaikie at gmail.com
Fri Feb 27 12:43:02 PST 2015


On Fri, Feb 27, 2015 at 12:35 PM, Frédéric Riss <friss at apple.com> wrote:

>
> On Feb 25, 2015, at 10:03 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Wed, Feb 25, 2015 at 9:46 PM, Frédéric Riss <friss at apple.com> wrote:
>
>>
>> On Feb 25, 2015, at 9:28 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> On Wed, Feb 25, 2015 at 9:20 PM, Duncan P. N. Exon Smith <
>> dexonsmith at apple.com> wrote:
>>
>>>
>>> > On 2015 Feb 25, at 21:14, David Blaikie <dblaikie at gmail.com> wrote:
>>> >
>>> >
>>> >
>>> > On Wed, Feb 25, 2015 at 7:15 PM, Duncan P. N. Exon Smith <
>>> dexonsmith at apple.com> wrote:
>>> > > On 2015 Feb 19, at 07:39, Frédéric Riss <friss at apple.com> wrote:
>>> > >
>>> > > ping ?
>>> > >
>>> > >> On Feb 12, 2015, at 11:52 AM, Frédéric Riss <friss at apple.com>
>>> wrote:
>>> > >>
>>> > >>>
>>> > >>> On Feb 6, 2015, at 10:28 AM, David Blaikie <dblaikie at gmail.com>
>>> wrote:
>>> > >>>
>>> > >>> Yeah, I'm punting on the language lawyering to Richard, hopefully…
>>> > >>
>>> > >> Richard,
>>> > >>
>>> > >> no need to look at the patch, but we just need you opinion on the
>>> following scenario:
>>> > >>
>>> > >> What the patch does is allocate all the class DIE objects using a
>>> BumpPtrAllocator to
>>> > >> be able to bulk deallocate them. This objects contain a SmallVector
>>> that in some rare
>>> > >> cases will resort to heap allocation to store their contents. I
>>> added bookkeeping to
>>> > >> keep track of the objects that need their destructors called to
>>> free the heap allocation.
>>> > >>
>>> > >> Basically the code is
>>> > >> std::vector<DIE *> DIEsToDelete
>>> > >> …
>>> > >> for (auto *DIE : DIEsToDelete)
>>> > >>    DIE->~DIE();
>>> > >>
>>> > >> There is a catch though. Some of these objects are of type DIEBlock
>>> or DIELoc which
>>> > >> use an identical multiple inheritance scheme:
>>> > >>
>>> > >> class DIEBlock : public DIEValue, public DIE {
>>> > >> …
>>> > >> };
>>> > >>
>>> > >> I’m storing a pointer to these in the above list and thus invoking
>>> just the DIE
>>> > >> destructor on them. The objects aren’t used afterwards as the next
>>> step is to
>>> > >> free the memory allocated by the BumpPtrAllocator.
>>> > >>
>>> > >> David was worried that this might be UB. Any definitive opinion?
>>> >
>>> > I'm not Richard, but I don't see anything in the standard to
>>> > suggest this is UB.  It seems like the relevant section of the
>>> > standard should be "12.4 Destructors", and explicit destructor
>>> > calls are described in point 12.
>>> >
>>> > > In an explicit destructor call, the destructor name appears as a ̃
>>> followed by a type-name or decltype- specifier that denotes the
>>> destructor’s class type. The invocation of a destructor is subject to the
>>> usual rules for member functions (9.3), that is, if the object is not of
>>> the destructor’s class type and not of a class derived from the
>>> destructor’s class type, the program has undefined behavior (except that
>>> invoking delete on a null pointer has no effect).
>>> >
>>> > I read through the rest of the section as well; I don't see any
>>> > indication that this is invalid.
>>> >
>>> > "12.7 Construction and destruction" also looks relevant, but I
>>> > can't find anything suspicious there either.
>>> >
>>> > @David, why do you suspect this is UB?
>>> >
>>> > Just seemed sufficiently suspicious that I was willing to punt to
>>> someone more authoritative, but I'll leave it to you guys if you reckon it
>>> seems reasonable.
>>>
>>> Okay, cool.  After my research today, I'm confident this is fine
>>> (famous last words, maybe) so I'll LGTM that part.
>>>
>>> I haven't looked at the patch itself recently... not sure if that
>>> was the only blocker?
>>>
>>
>> I hadn't really looked at it in any great detail personally. It is some
>> unfortunate added complexity, but I haven't given thought to
>> alternatives/improvements.
>>
>>
>> I’m happy to discuss alternatives if anyone has ideas. I went to  quite a
>> few iterations before submitting this patch and the one I posted is by far
>> the simplest of what I tried.
>>
>
> Any brief summary of some of the options you considered/tried? 'tis useful
> to understand how you/we end up where we end up so someone doesn't waste
> time trying something you've already considered (either now or later)
>
>
> I somehow missed your reply and I was going to complain that it would be
> nice for my dsymutil work to land that soon :-/
>
> Let’s start with what the patch does:
>   - it allocates all DIEs from the same BumpPtrAllocator the DIEValues are
> allocated from.
>   - it moves the children list from a vector to a ilist (you seem to imply
> there is an issue with that, but I didn’t see any).
>

There was some complexity in moving to std::list, but probably not ilist
(because ilist is intrusive - so if you dynamically allocate the object up
front (& transfer ownership into the list) you don't have issues with
invalidation/pointer validity - with std::list you can only keep pointers
to the nodes (so only take pointers once the thing is in the list, not
before) and if you splice them around, never taking them out of a list to
put them into another)


>   - it adds a bookkeeping to track the DIEValue and Abbreviation
> SmallVectors that grow over there inline size. The bookkeeping structure is
> passed as optional parameter to the methods that might grow the
> SmallVectors.
>
> The first 2 items have been basically the same in all my experiments. The
> third one can’t just be moved to a list variant because at least one
> DIEValue is shared among multiple DIEs.
>

I think we just pool the integer value 1 (or zero) die value - maybe that's
not worth pooling for the added complexity it sounds like it maybe bringing?


>
> Here’s what I remember trying for the 3rd issue: I tried to use
> std::vectors for the attributes with a custom stateful allocator. That
> allocator would get its memory from a BumpPtrAllocator and use some simple
> recycling logic to reuse the memory relinquished by growing vectors. This
> works because vectors all have the same growth factor (although that’s not
> guaranteed by the standard I suppose). The performance wasn’t as good as
> with this patch and it makes the DIEs bigger as they need to store a
> reference to the allocator state. I guess the perf difference comes from a
> combination of added memory pressure and losing the SmallVector’s  inline
> optimization. Adding calls to reserve() wouldn’t help that much. I can’t
> remember the exact numbers, but there was a meaningful difference, and I’m
> not sure making the DIE object bigger is a good idea anyway. (To mitigate
> the DIE object size increase, I also tried to store the DIEValues and
> Abbrevs in the same vector. To my surprise this did actually result in even
> lower performance).
>
> One other very simple thing that I tried is to not do any bookkeeping, but
> to walk the DIE tree at finalization time and call the destructors of all
> the DIEs that had heap allocated there SmallVectors. Not very cache
> friendly.
>

Why would that be more cache unfriendly? You'll be visiting each of the
SmallVectors anyway... oh, I suppose at least when you add the vectors as
they're built to the list of things to cleanup you'll be more likely to
find co-located vectors next to each other. *shrug* maybe.


> The performance was 20 to 30% lower than with this patch.
>
> Fred
>
> (FWIW, though I doubt it'd help any, I did at one point consider changing
> the child vector<unique_ptr<DIE>>s into list<DIE>s (or maybe even
> forward_list<DIE>s probably - but it was a bit tricky to make sure the DIEs
> were already in the lists before they were pointed to (same sort of problem
> Adrian had with the cross-CU reference stuff, sort of)) - I suppose maybe
> if that was feasible we could use custom allocators for the lists & that
> would gain similar performance? (but still have to plumb through the
> allocators... ) - actually, changing to lists would remove the need for the
> small vector optimization. Since we're allocating memory for each node
> anyway & we don't need random child access)
>
>
>> The patch in itself is quite critical for dsymutil performance, but it
>> also shaves 2% on debug builds which IMO is nice.
>>
>> Fred
>>
>>
>>> > Any reason not to trust
>>> > the UBSan bots to catch problems?
>>> >
>>> > I don't really expect UBSan to be complete, though I don't know the
>>> specific set of checks it does have - so I usually assume it won't catch
>>> particular bugs unless I'm specifically aware of its support for that bug
>>> type. Happy to be corrected/enlightened.
>>>
>>> Fair point!
>>>
>>> >
>>> > - David
>>> >
>>> >
>>> > >>
>>> > >>> On Thu, Feb 5, 2015 at 9:25 AM, Frédéric Riss <friss at apple.com>
>>> wrote:
>>> > >>> ping?
>>> > >>>
>>> > >>>> On Jan 27, 2015, at 6:43 PM, Frédéric Riss <friss at apple.com>
>>> wrote:
>>> > >>>>
>>> > >>>>>
>>> > >>>>> On Jan 27, 2015, at 4:11 PM, David Blaikie <dblaikie at gmail.com>
>>> wrote:
>>> > >>>>>
>>> > >>>>>
>>> > >>>>>
>>> > >>>>> On Tue, Jan 27, 2015 at 2:08 PM, Frederic Riss <friss at apple.com>
>>> wrote:
>>> > >>>>> This update removes the limitation to a fixed maximum number of
>>> > >>>>> attributes per DIE. The patch is very similar to the first one
>>> that
>>> > >>>>> I sent for discussion. It only adds a pointer to a vector of DIEs
>>> > >>>>> as arguement to DIE::addValue(). If a DIE overflows its inline
>>> storage
>>> > >>>>> for attributes, it is added to the vector. The vector is then
>>> iterated
>>> > >>>>> just before the BumpPtrAllocator is destroyed to call the
>>> destructors
>>> > >>>>> of all these DIEs.
>>> > >>>>>
>>> > >>>>> This approach gives the most flexibilty as it doesn't impose the
>>> > >>>>> memory management upon the user (for example DIEHashTest.cpp
>>> continues
>>> > >>>>> to use stack allocated DIEs and it works fine).
>>> > >>>>>
>>> > >>>>> I've been playing with this for the last few days, and I have
>>> quite
>>> > >>>>> a few different implementations lying around (for example using
>>> > >>>>> std::vectors with a custom stateful allocator). They are all more
>>> > >>>>> complicated and don't perform as well as this patch.
>>> > >>>>>
>>> > >>>>> There's one point that I'd like to mention: the patch unifies the
>>> > >>>>> existing bookkeeping of the DIEBlock and DIELoc objects and
>>> treats
>>> > >>>>> them the same as standard DIEs. This means that we will call ~DIE
>>> > >>>>> on DIEBlock and DIELoc objects which will result in a 'partial'
>>> > >>>>> destruction (DIEBlock inherits both from DIEValue and from DIE).
>>> > >>>>> I think this is fine. It should do exactly what we want and just
>>> > >>>>> call the destructors of the SmallVectors in the DIE part of the
>>> > >>>>> object, but I wanted to mention it in case someone thinks its not
>>> > >>>>> legal to do that.
>>> > >>>>>
>>> > >>>>> Pretty sure that's not valid C++ (not sure quite which cracks
>>> the bump allocator objects fall in general in terms of not running dtors,
>>> reusing memory, etc - and whether this would be worse than that or not,
>>> though).
>>> > >>>>
>>> > >>>> What exactly do you think the issue would be from a language
>>> standpoint? Calling a destructor on a ‘partial’ object? I would expect that
>>> it is ok to do so. AFAIK, explicit destructor calls follow the same rules
>>> as other function calls. I have a pointer to a DIE object, thus I can call
>>> the destructor on it. The object stops to ‘exist’ at that point, and thus
>>> its enclosing object is in an undefined state, but this doesn’t really
>>> matter as it won’t be touched anymore.
>>> > >>>>
>>> > >>>> I’m very bad at language lawyering though, thus I’d really
>>> appreciate a authoritative answer :-)
>>> > >>>>
>>> > >>>> Fred
>>> > >>>>
>>> > >>>>>
>>> > >>>>>
>>> > >>>>> http://reviews.llvm.org/D7072
>>> > >>>>>
>>> > >>>>> Files:
>>> > >>>>>   include/llvm/CodeGen/DIE.h
>>> > >>>>>   lib/CodeGen/AsmPrinter/DIE.cpp
>>> > >>>>>   lib/CodeGen/AsmPrinter/DIEHash.cpp
>>> > >>>>>   lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
>>> > >>>>>   lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
>>> > >>>>>   lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>>> > >>>>>   lib/CodeGen/AsmPrinter/DwarfFile.cpp
>>> > >>>>>   lib/CodeGen/AsmPrinter/DwarfUnit.cpp
>>> > >>>>>   lib/CodeGen/AsmPrinter/DwarfUnit.h
>>> > >>>>>   unittests/CodeGen/DIEHashTest.cpp
>>> > >>>>>
>>> > >>>>> EMAIL PREFERENCES
>>> > >>>>>   http://reviews.llvm.org/settings/panel/emailpreferences/
>>> > >>>>
>>> > >>>> _______________________________________________
>>> > >>>> llvm-commits mailing list
>>> > >>>> llvm-commits at cs.uiuc.edu
>>> > >>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>> > >>>
>>> > >>>
>>> > >>
>>> > >> _______________________________________________
>>> > >> llvm-commits mailing list
>>> > >> llvm-commits at cs.uiuc.edu
>>> > >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>> > >
>>> > > _______________________________________________
>>> > > llvm-commits mailing list
>>> > > llvm-commits at cs.uiuc.edu
>>> > > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150227/81e058f6/attachment.html>


More information about the llvm-commits mailing list