RFC: Reduce the memory footprint of DIEs (and DIEValues)
Duncan P. N. Exon Smith
dexonsmith at apple.com
Tue May 26 20:03:58 PDT 2015
> On 2015 May 26, at 17:54, Frédéric Riss <friss at apple.com> wrote:
>
> Some nits on the final patch. Mostly style things, feel free to address or not… (especially the llvm-dsymutil stuff. I commented it for reference/discussion, but I can definitely address that myself as a followup)
Thanks for the review! I'll incorporate some of this as I go, and some as
follow-ups (post-commit).
All your comments SGTM, but a few responses inline below.
>
>> --- a/include/llvm/CodeGen/DIE.h
>> +++ b/include/llvm/CodeGen/DIE.h
>
> Given the magnitude of the change, it might be the right time to update the
> comment style in this file.
>
>> +/// A list of DIE values.
>> +///
>> +/// This is a singly-linked list, but instead of reversing the order of
>> +/// insertion, we keep a pointer to the back of the list so we can push in
>> +/// order.
>> +class DIEValueList {
>> + struct Deleter {
>> + void operator()(void *) {}
>> + };
>> + struct Node {
>> + typedef std::unique_ptr<Node, Deleter> UniquePtr;
>
> Although the Deleter code is very close, wouldn’t it be better to give it
> a more descriptive name, like ‘DoNothingDeleter’ (I’m really bad at names).
In my commit messages I've been calling this a "null deleter" so I'll
change this to `NullDeleter`.
>
>
>> + /// Children DIEs.
>> + ///
>> + /// Children are stored in an intrusively linked list.
>> + ///
>> + /// TODO: Allocate DIEs on a BumpPtrAllocator to avoid malloc traffic.
>
> This TODO should go away now
>
>
>> + // Accessors.
>> + const DIEAbbrev &getAbbrev(const DwarfFile &F) const;
>
> I find this interface a bit strange. The caller might as well call getAbbrev on
> DwarfFile himself. I’d like to avoid more ties between the Dwarf* Codegen
> machinery and the DIE objects.
> I might be wrong, but I think it’s unused anyway.
Eric commented on the line of code that I *meant* to change to
call this function. I guess I left both sides awkward, and one
unused :/. I'll clean this up somehow.
>
>
>
>> + /// findAttribute - Find a value in the DIE with the attribute given,
>> + /// returns NULL if no such attribute exists.
>> + DIEValue findAttribute(dwarf::Attribute Attribute) const;
>
> The comment seems out of date, this cannot return null. Wouldn’t it be
> cleaner for this to return a value_iterator anyway? (further down the patch,
> I’ve seen the use of this inside a if condition. Makes a lot of sense! The
> comment is still wrong though :-))
>
>
>> +DIE::UniquePtr DIE::reverseChildren(UniquePtr Child, UniquePtr Reversed) {
>> + if (!Child)
>> + return Reversed;
>> +
>> + UniquePtr Next = std::move(Child->NextSibling);
>> + Child->NextSibling = std::move(Reversed);
>> + return reverseChildren(std::move(Next), std::move(Child));
>> +}
>
> I might have missed it above, but shouldn’t we check that the children list has
> been reversed in the children() accessor?
Yes! Good idea.
> Or maybe we iterate the children
> somewhere before finalizing the DIE?
Nope (or else my patch is incorrect, and we need to make them in
order to begin with).
> On a related note, couldn’t we find someplace
> canonical to do the DIE finalization rather than calling it manually? (I can’t
> think of one that fits all users, but others might have an idea)
I couldn't find one when I looked.
Another option is to spend a pointer on tracking the "last" child
(like I did in `DIEValueList`) so we can push_back() instead of
push_front(). I'd prefer to save the pointer if we can, though.
(Frankly, I'd like to remove the `Parent` pointer -- you can change
`NextSibling` to `NextSiblingOrParent` (pointing at the Parent if
it's the last sibling) and pack a `bool` in -- but someone (Pete?
Fred?) pointed out that `getParent()` is in a hot path (we're
walking up to the root DIE for something somewhere...).
(On that subject, I'd like to remove the `Last` pointer in
`DIEValueList`, but we *do* iterate through it before adding all the
nodes so it's not obvious how to do it efficiently without changing
the output. (Actually, I just thought of something: (1) make it a
circular linked list, (2) point at Last instead of First, and (3)
add a `bool` to indicate "last-ness" instead of checking for null.
Seems obvious now. Maybe I'll do this in a follow-up (saves a
pointer in `DIEValueList` and negates the need to reverse children
in `DIE`, at the cost of no longer being able to use
`std::unique_ptr<..., NullDeleter>`.)))
>
>>
>> const TargetLoweringObjectFile &TLOF = Asm->getObjFileLowering();
>> - addSectionLabel(UnitDie, dwarf::DW_AT_stmt_list, LineTableStartSym,
>> - TLOF.getDwarfLineSection()->getBeginSymbol());
>> + StmtListIndex =
>> + addSectionLabel(UnitDie, dwarf::DW_AT_stmt_list, LineTableStartSym,
>> + TLOF.getDwarfLineSection()->getBeginSymbol());
>
> The variable name should be changed, this isn’t an index anymore.
>
>
>
>> --- a/tools/dsymutil/DwarfLinker.cpp
>> +++ b/tools/dsymutil/DwarfLinker.cpp
>> @@ -60,6 +60,25 @@ using HalfOpenIntervalMap =
>>
>> typedef HalfOpenIntervalMap<uint64_t, int64_t> FunctionIntervals;
>>
>> +struct PatchLocation {
>> + DIE::value_iterator I;
>
> Still not fond of the name. I might just change it at some point to just
> pass a value_iterator everywhere, but I’ll try that myself.
Not hard for me to remove this at the end; `PatchLocation` was mainly
a useful bridge between 0002 and 0007. I'll clean this up.
>
>
>> @@ -1793,7 +1813,7 @@ unsigned DwarfLinker::cloneDieReferenceAttribute(
>> assert(Ref > InputDIE.getOffset());
>> // We haven't cloned this DIE yet. Just create an empty one and
>> // store it. It'll get really cloned when we process it.
>> - RefInfo.Clone = new DIE(dwarf::Tag(RefDie->getTag()));
>> + RefInfo.Clone = DIE::get(DIEAlloc, dwarf::Tag(RefDie->getTag())).get();
>> }
>> NewRefDie = RefInfo.Clone;
>
> The non-deleting unique_ptr makes this look strange (having a temporary
> unique_ptr one which you just call get() is usually pretty bad).
Hah! Didn't notice this. Should definitely be `release()`.
> I don’t see a nice way to make the ownership clearer though. RefInfo.Clone
> cannot be a unique_ptr, as at some point it’ll get moved into another’s
> DIE children list, but it might still be referenced after that.
Yeah the ownership in DwarfLinker is a little awkward. Let's talk in
person; I have a few ideas about how to simplify it. But I know that
tool is still a WIP, so I imagine a lot of it will get cleaned up as
you go.
>
>>
>> + auto Stmt = std::find_if(OutputDIE->begin_values(), OutputDIE->end_values(),
>> + [](const DIEValue &Value) {
>> + return Value.getAttribute() == dwarf::DW_AT_stmt_list;
>> + });
>
> This could be replaced by the new findAttribute().
>
>
> Again, awesome patch!
> Thanks,
> Fred
>
>> On May 26, 2015, at 3:30 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>
>> bcc:llvmdev, +llvm-commits
>>
>> Thanks Eric!
>>
>> David, let me know if you have any comments on the `DIE::Children`
>> stuff (patch 0008 in particular, although I think Eric wanted your
>> opinion on 0007 as well).
>>
>> Fred, you mentioned in person you want to have a look. I'll hold
>> off on committing until you have a chance too.
>>
>> I've rebased the patches, removed some FIXMEs that I forgot to
>> delete, and added an 'all.patch' in case that's more convenient.
>>
>> (I haven't addressed Eric's comments yet; I'm mainly reposting to
>> move this off of llvmdev.)
>>
>> <0001-AsmPrinter-Reorganize-DIE.h-NFC.patch><0002-AsmPrinter-Change-DIEValue-to-be-stored-by-value.patch><0003-AsmPrinter-Store-abbreviation-data-directly-in-DIE-a.patch><0004-AsmPrinter-Stop-exposing-underlying-DIEValue-list-NF.patch><0005-AsmPrinter-Return-added-DIE-from-DIE-addChild.patch><0006-AsmPrinter-Stop-exposing-underlying-DIE-children-lis.patch><0007-AsmPrinter-Convert-DIE-Values-to-a-linked-list.patch><0008-AsmPrinter-Use-an-intrusively-linked-list-for-DIE-Ch.patch><all.patch>
>>
>>> On 2015-May-26, at 14:41, Eric Christopher <echristo at gmail.com> wrote:
>>>
>>> Hi Duncan,
>>>
>>> Few random comments on things, in general looks pretty good.
>>>
>>> 0001 - LGTM.
>>> 0002 - LGTM.
>>> 0003 -
>>>
>>> struct AttrEntry {
>>> DIEValue Val;
>>> - const DIEAbbrevData *Desc;
>>> };
>>>
>>> Most boring struct ever?
>>>
>>> + const DIEAbbrev &Abbrev = getAbbrev(Die.getAbbrevNumber());
>>>
>>> This is a bit of an awkward construction. I understand where it's coming from, but can you go back and comment the various abbreviation bits with how it works now?
>>>
>>> - AssignAbbrev(Die->getAbbrev());
>>> + AssignAbbrev(NewAbbrev);
>>> + Die->setAbbrevNumber(NewAbbrev.getNumber());
>>>
>>> To elaborate - the need for this sort of thing should be documented.
>>>
>>> 0004 - LGTM.
>>> 0005 - Better commit message saying why you're going to want this more later?
>>> 0006 - LGTM.
>>> 0007 - LGTM, might want to have Dave take a look at it as well.
>>> 0008 - reverseChildren and finalizeChildren is pretty gross. Also, I'm going to punt on the rest of this to Dave.
>>>
>>> Thanks for all the work!
>>>
>>> -eric
>>>
>>> On Sun, May 24, 2015 at 11:55 AM Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>>
>>>> On 2015 May 20, at 17:51, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>>>
>>>>
>>>>> On 2015 May 20, at 17:39, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>>>>
>>>>> With just those four patches, memory usage went *up* slightly. Add in
>>>>> the 5th patch (which does #2 below), and we get an overall memory drop
>>>>> of 4%.
>>>>
>>>> Forgot to post numbers for this. Peak memory was at 920 MB before
>>>> the five patches, and 884 MB after. (These exact numbers won't quite
>>>> reproduce in ToT since I still haven't finished cleaning up and
>>>> committing the MCSymbol and emitLabelDiff() work I hacked on top of,
>>>> but the 36 MB drop should hold.)
>>>
>>> I've cleaned all this up and committed the most obvious parts, as well
>>> as a few unrelated memory improvements. I'm attaching my (almost?)
>>> ready-to-go patches, which have the following effects on peak memory:
>>>
>>> - 0000: 845 MB (baseline)
>>> - 0001: 845 MB - refactor
>>> - 0002: 879 MB - pass DIEValue by value (momentary setback)
>>> - 0003: 829 MB - merge DIEAbbrevData into DIEValue
>>> - 0004: 829 MB - refactor
>>> - 0005: 829 MB - refactor
>>> - 0006: 829 MB - refactor
>>> - 0007: 764 MB - change DIE::Values to a linked list
>>> - 0008: 756 MB - change DIE::Children to a linked list
>>>
>>> (Still measuring memory on `llc` for `-flto -g`; details in r236629.)
>>>
>>> @Eric, you mentioned offline you'd like to have a look at this proposal
>>> before I proceed -- obviously I've been impatient ;). Let me know if
>>> I'm okay to move forward and start committing (modulo a couple of these
>>> that I'll want a review from David and Fred on).
>>>
>>
>
More information about the llvm-commits
mailing list