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

David Blaikie dblaikie at gmail.com
Wed Feb 25 22:03:00 PST 2015


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)

(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/20150225/86108182/attachment.html>


More information about the llvm-commits mailing list