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

David Blaikie dblaikie at gmail.com
Fri Feb 27 13:34:14 PST 2015


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

>
> On Feb 27, 2015, at 12:43 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> 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?
>
>
> This is something I didn’t try, however: There are potentially a lot of
> DIEs, but there is an order of magnitude more Abbrev+DIEValues. Adding The
> memory overhead of the ilist to those would be a bad idea IMO.
>
>
>> 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.
>
>
> I just meant that walking the whole DIE tree at finalization time is very
> cache unfriendly. With this patch I only walk the vector of DIEs to delete.
> And there are very few of them.
>

Ah, right right.


>
> Fred
>
> 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/bc92e4a3/attachment.html>


More information about the llvm-commits mailing list