RFC: Reduce the memory footprint of DIEs (and DIEValues)
Frédéric Riss
friss at apple.com
Tue May 26 17:54:48 PDT 2015
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)
> --- 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).
> + /// 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.
> + /// 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? Or maybe we iterate the children
somewhere before finalizing the DIE? 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)
>
> 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.
> @@ -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).
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.
>
> + 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