<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun 25, 2015 at 3:21 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
> On 2015-Jun-25, at 15:13, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
> Looks good (might eventually iterate a bit on the generic list data structures (names, API, etc) - but they're factored out enough)<br>
><br>
> Does DIE::get need to be templated on the allocator type?<br>
<br>
</span>Nope, that's from when I was prototyping, just missed that site.<br>
(I was using a `MallocAllocator` and leaking everything so that I<br>
could get better information from the heap profile). I'll fix that<br>
and commit.<br></blockquote><div><br>Thanks<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
><br>
> On Thu, Jun 25, 2015 at 2:57 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
><br>
> > On 2015-Jun-25, at 13:13, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> ><br>
> ><br>
> ><br>
> > On Thu, Jun 25, 2015 at 1:01 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
> ><br>
> > > On 2015-Jun-25, at 12:44, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> > ><br>
> > ><br>
> > ><br>
> > > On Thu, Jun 25, 2015 at 12:04 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
> > ><br>
> > > > On 2015-Jun-25, at 12:00, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> > > ><br>
> > > ><br>
> > > ><br>
> > > > On Thu, Jun 25, 2015 at 11:49 AM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
> > > ><br>
> > > > > On 2015-Jun-25, at 10:48, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> > > > ><br>
> > > > > I'm not sure of the merits of handing around unique_ptrs with a null deleter (I could see it in some generic template machinery - but this is pretty explicitly never-owning). I get the desire to tag the conceptual ownership, and I appreciate that - but without any checking I suspect it'll just become incorrect over time and we won't notice.<br>
> > > ><br>
> > > > IMO, where tagging the ownership is valuable is before the DIE is<br>
> > > > inserted in the tree (and in this patch that's the only place it<br>
> > > > remains). But maybe `unique_ptr` is kind of a funny name for what's<br>
> > > > going on.<br>
> > > ><br>
> > > > Maybe a way to keep it from bitrotting would be to make some of the<br>
> > > > API private, something like:<br>
> > > ><br>
> > > > class OwnedDIE {<br>
> > > > friend class DIE;<br>
> > > > DIE *Die = nullptr;<br>
> > > ><br>
> > > > OwnedDIE(const OwnedDIE &) = delete;<br>
> > > > OwnedDIE &operator=(const OwnedDIE &) = delete;<br>
> > > ><br>
> > > > OwnedDIE(DIE *Die) : Die(Die) {}<br>
> > > > DIE *release() {<br>
> > > > auto *D = Die;<br>
> > > > Die = nullptr;<br>
> > > > return D;<br>
> > > > }<br>
> > > ><br>
> > > > public:<br>
> > > > OwnedDIE() = default;<br>
> > > > ~OwnedDIE() = default;<br>
> > > > OwnedDIE(OwnedDIE &X) : Die(X.Die) { X.Die = nullptr; }<br>
> > > > OwnedDIE &operator=(OwnedDIE &X) {<br>
><br>
> (I meant `&&` for both of these :/.)<br>
><br>
> > > > Die = X.Die;<br>
> > > > X.Die = nullptr;<br>
> > > > return *this;<br>
> > > > }<br>
> > > > DIE *get() const { return Die; }<br>
> > > ><br>
> > > > As soon as you've got 'get()' (yeah, op* could be abused, but that's a bit more expliict work to take the address of the reference and go stash that somewhere) I think it becomes pretty easy to accidentally duplicate or otherwise play fast-and-loose with ownership.<br>
> > ><br>
> > > FWIW, it's impossible to play fast and loose between `DIE::get()` and<br>
> > > `DIE::addChild()` with this API. You can only mess around with<br>
> > > top-level DIEs. I'm not sure that's worth much though?<br>
> > ><br>
> > > I guess the only thing preventing you from adding the same DIE as a child to multiple nodes is the parental check? So the parent pointer acts as a sort of intrusive ownership tracking mechanism... :s doesn't exactly fill me with confidence.<br>
> ><br>
> > Right. (Unless we keep something like an `OwnedDIE`.)<br>
> ><br>
> > *nod* but without checking, still easy to abuse an OwnedDIE.<br>
> ><br>
> > Anyway, rather than going around in circles here: Without any checks (except the incidental one of the parent pointer) it feels misleading to talk about ownership. I would suggest/prefer abandoning the concept and thinking about it as the BumpPtrAllocator owning these allocations.<br>
> ><br>
> > But I leave that up to you - keep it the way it is if you prefer.<br>
><br>
> David and I talked offline and stopped circling each other, and then<br>
> Fred and I talked about the implications for dsymutil. Basically,<br>
> changing to the strict API I proposed above would require double<br>
> pointers in places in `dsymutil` -- raw pointer + container that<br>
> could be moved -- and there isn't really much benefit.<br>
><br>
> I'll add a note in the commit message that gives some background,<br>
> but it seems prudent just to admit these are bumpptr-allocated.<br>
><br>
> *idea, dropped*<br>
><br>
> David, how do the updated patches look? (Note, I committed r240701<br>
> in the meantime.)<br>
><br>
><br>
><br>
> ><br>
> > > That said, since they're bump pointer allocated anyway, there's no reason we couldn't have a multi-parented DIE anyway. (I mean it'd be silly because it'd print lots of redundant DWARF, but... *shrug*)<br>
> ><br>
> > Well, the `DIE::Offset` is stored intrusively in a `DIE`, so this<br>
> > isn't quite legal.<br>
> ><br>
> > Ah, right right.<br>
> ><br>
> > Also, `DIEHash::computeTypeSignature()` requires walking the parent<br>
> > list -- otherwise, we could almost get rid of it (there are a<br>
> > couple of other users, but IIRC they're not as hard to refactor).<br>
> ><br>
> > Hmm - I thought one of them was the stuff used to do cross-DIE referencing which was pretty hard or ugly to do in other ways, if I remember the rather long review that went on for that change...<br>
> ><br>
> > But if it's for types in particular - we only produce type signatures when producing the CU signature (we don't use type signatures for type units) and so we can probably keep track of the parent chain during the hash walk and re-use it rather than walking up it.<br>
><br>
> Hmm; if you can get rid of the need in type signatures, maybe I'll<br>
> have a closer look at the other uses. They seemed tractable to me.<br>
><br>
> The only other one I wasn't sure about was a place where we need to<br>
> look up the unit (by finding the *final* node in the list). If I<br>
> can't get rid of it entirely, we could at least rename `Parent` to<br>
> `Unit` and save the walk.<br>
><br>
><br>
> ><br>
> ><br>
> > ><br>
> > > > DIE &operator*() const { return *Die; }<br>
> > > > DIE *operator->() const { return Die; }<br>
> > > > explicit operator bool() const { return Die; }<br>
> > > > };<br>
> > > ><br>
> > > > class DIE {<br>
> > > > public:<br>
> > > > // Calls OwnedDIE::OwnedDIE(DIE *).<br>
> > > > static OwnedDIE get(BumpPtrAllocator &Alloc, dwarf::Tag Tag);<br>
> > > > // Calls OwnedDIE::release().<br>
> > > > DIE &addChild(OwnedDIE Child);<br>
> > > > };<br>
> > > ><br>
> > > > I guess I'm not sure how much better this is, although it would<br>
> > > > prevent llvm-dsymutil from compiling without some more<br>
> > > > reorganization (currently it calls `release()` and recreates<br>
> > > > `unique_ptr<>` between functions).<br>
> > > ><br>
> > > > > If we're going to keep it - could we add some checking in debug builds somehow? (skip the bump ptr allocation entirely in debug build? use a typed bump ptr allocator (it calls dtors) in a debug build? Just a side table hash set to check construction/destruction pairs and an assert that it's empty on exit?)<br>
> > > ><br>
> > > > The `OwnedDIE` idea above adds a bit more safety, but it wouldn't<br>
> > > > catch "leaks", and a consumer (like dsymutil) and a consumer could<br>
> > > > choose to store top-level DIEs outside an `OwnedDIE` container.<br>
> > > ><br>
> > > > Thoughts?<br>
> > > ><br>
> > > > Here's a claim I'll try on for size:<br>
> > > ><br>
> > > > Either we should have a mechanism that enforces ownership-like semantics (which I think boils down to: checked pseudo-destruction in asserts builds) or we probably should just forget about it and treat these things as raw pointers with no specific sense of ownership. Just accept that they were all created with the BumpPtrAllocator (perhaps make the API restrictions stronger in this regard? delete op new/delete? I forget how all that works) and move on.<br>
> > ><br>
> > > Hmm, I guess we already check the `Parent` pointer when adding<br>
> > > children, so maybe you're right.<br>
> > ><br>
> > > ><br>
> > > > > Was there a reason to squash the DIEValue and DIE allocator changes together? (probably related to the DIEBlock? (both a value and a DIE) - I thought I remembered reading something from you about changing/splitting that? ah, that's your other patch - if that came first then we could split the allocation changes for DIEValue and DIE into separate patches? But that's too much work to rejig it all?)<br>
> > > ><br>
> > > > I think I can split the DIEValue/DIE changes. New patches early<br>
> > > > afternoon I hope.<br>
> > > ><br>
> > > > ><br>
> > > > > in DwarfLinker::patchLineTableForUnit why the change back to a range-for from std::find_if?<br>
> > > ><br>
> > > > I don't remember =/. I'll see if I can get the original flow working,<br>
> > > > and if I can't I'll know why I changed it.<br>
> > > ><br>
> > > > ><br>
> > > > > (I do get tired of seeing allocators passed in/around everywhere - would it make more sense to wrap the allocator in a type with utility functions for generating things? I suppose it's no great win, but feels a bit better, maybe? eg: "DIE::get(DIEValueAllocator, dwarf::DW_TAG_inlined_subroutine);" -> "DIEBuilder.get(dwarf::DW_TAG_inlined_subroutine)" or similar - for values it's trickier, the DIE and the allocator are both needed every time, but there's no easy access to the allocator from values or dies... *shrug* )<br>
> > > ><br>
> > > > Maybe I'll play with some builder ideas, see if it reduces the churn<br>
> > > > at all? We could have:<br>
> > > ><br>
> > > > DIEBuilder DieB(Alloc);<br>
> > > > auto Die = DieB.get(Tag);<br>
> > > > DIEValueBuilder DieValueB = DieB.addValues(Die);<br>
> > > > DieValueB.addValue(SomeDieValue);<br>
> > > ><br>
> > > > I'm not sure this will actually be better in practice though... I'll<br>
> > > > play around after posting new patches.<br>
> > > ><br>
> > > > Yeah - don't worry too much if it doesn't look useful. And it can be orthogonal cleanup if we one day discover a great API. But I appreciate giving it a bit of thought.<br>
> > > ><br>
> > > > - David<br>
> > > ><br>
> > > ><br>
> > > > > On Wed, Jun 24, 2015 at 1:11 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
> > > > ><br>
> > > > > > On 2015 Jun 17, at 11:36, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> > > > > ><br>
> > > > > >> On Wed, Jun 17, 2015 at 11:26 AM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
> > > > > >><br>
> > > > > >> > On 2015 Jun 16, at 21:40, Yaron Keren <<a href="mailto:yaron.keren@gmail.com">yaron.keren@gmail.com</a>> wrote:<br>
> > > > > >> ><br>
> > > > > >> > Hi Duncan,<br>
> > > > > >> ><br>
> > > > > >> > Singly linked lists are used throught LLVM: ArrayRecycler, Registry, LiveInterval, SCEVUnknown, CGBlockInfo, MacroArgCache, ...DIEValueList.<br>
> > > > > >> ><br>
> > > > > >> > What do you think about implementing a singly-linked ADT?<br>
> > > > > >> ><br>
> > > > > >><br>
> > > > > >> Interesting idea.<br>
> > > > > >><br>
> > > > > >> I've taken a look at the other singly-linked lists you mentioned. Most<br>
> > > > > >> of these are some version of a free/recycling list (ArrayRecycler,<br>
> > > > > >> Registry, MacroArgCache, etc.), which need super simple push/pop API and<br>
> > > > > >> no memory management.<br>
> > > > > >><br>
> > > > > >> LiveInterval and CGBlock also need iteration and a way to conditionally<br>
> > > > > >> remove elements (something like `remove_if()`), and in a few cases it<br>
> > > > > >> would be convenient to have API to destruct and/or destroy nodes.<br>
> > > > > >><br>
> > > > > >> I think these would all be straightforward to formalize into an ADT. We<br>
> > > > > >> could even use it to implement a value-based `llvm::forward_list<>` that<br>
> > > > > >> avoided the sorting bug in libstdc++ (which gets "stable" exactly<br>
> > > > > >> backwards) that David hit when refactoring tablegen.<br>
> > > > > >><br>
> > > > > >> Unfortunately, I think it'd be hard to reuse for DIE::Children and<br>
> > > > > >> DIEValueList. The latter wants to preserve insertion order and needs<br>
> > > > > >> `push_back()` API (the former can technically get away with a timely<br>
> > > > > >> call to `reverse()`, but it's awkward). Supporting `push_back()`<br>
> > > > > >> efficiently requires pointing at the last element somehow, either via an<br>
> > > > > >> extra pointer or circular logic (what my latest patch does for<br>
> > > > > >> DIE::Children).<br>
> > > > > ><br>
> > > > > > One of the other problems I hit when trying to value-ify the DIE children is that the point of DIE creation is, in some particular cases, divorced from the knowledge of which container it's going to go in to. Yet pointer identity is needed before then.<br>
> > > > > ><br>
> > > > > > At least that's how I recall it - may be worth going down that path again just so we can write it up clearly for posterity, I dunno.<br>
> > > > ><br>
> > > > > Nope, I saw this too.<br>
> > > > ><br>
> > > > > In theory, you could get std::list to work here, by inserting into a<br>
> > > > > singleton linked list when you first need the reference, and splicing<br>
> > > > > it into its parent's linked list once it's created. However, I think a<br>
> > > > > value-based list is too expensive anyway (see below)...<br>
> > > > ><br>
> > > > > > If it is possible, then I would love/prefer to use an existing non-intrusive container (or writing one if we have to). at least could prototype with std::list, then, as you say, implement a std::forward_list-like device with the bonus that we could fix sort and do the circular trick to allow insertion at the end, etc.<br>
> > > > ><br>
> > > > > Unfortunately the BumpPtrAllocator/skip-unlinking-the-list part of these<br>
> > > > > patches is really necessary, making std::list pretty awkward. My payload<br>
> > > > > usually takes ~2 minutes when profiling, and when I profiled linked lists<br>
> > > > > without the BumpPtrAllocator it was talking >15 minutes to teardown (I<br>
> > > > > killed it, so I don't know how much longer it would have taken).<br>
> > > > ><br>
> > > > > Moreover, using a BumpPtrAllocator this way fixes the slow DIE teardown<br>
> > > > > that Fred was battling in llvm-dsymutil.<br>
> > > > ><br>
> > > > > > (& sorry, will get to proper review of this patch soon... )<br>
> > > > ><br>
> > > > > (Sadly, I completely missed this reply of yours until this morning (when<br>
> > > > > I was already preparing the patches below).)<br>
> > > > ><br>
> > > > > I've rebased the patches on top of r240566, and squashed them together<br>
> > > > > with shared implementation for the two linked lists in die-diet.patch.<br>
> > > > ><br>
> > > > > Yesterday (on top of 240244), I reran llc to check memory usage on the<br>
> > > > > verify-uselistorder.lto.opt.bc payload. This patch drops peak memory<br>
> > > > > usage from 800MB down to 720MB, 10%. I also timed before/after the patch<br>
> > > > > (no heap profile, CPU speed locked down, best of 5 runs) and measured a<br>
> > > > > 1% speedup.<br>
> > > > ><br>
> > > > ><br>
> > > > ><br>
> > > > ><br>
> > > > > (I've also attached die-block-is-not-a-die.patch, which fixes the<br>
> > > > > DIELoc/DIEBlock/DIE relationship. I've been holding off on committing<br>
> > > > > it since it's built on top of the die-diet.patch.<br>
> > > > ><br>
> > > > ><br>
> > > > ><br>
> > > > > )<br>
> > > > ><br>
> > > > > ><br>
> > > > > >> We could configure this stuff via templates -- I'd be open to the idea<br>
> > > > > >> -- but I think the intersection between the implementations would be<br>
> > > > > >> practically nil. Even the iterator implementations need to be<br>
> > > > > >> completely different. If these are the only `push_back()`-enabled<br>
> > > > > >> slists in tree, is it premature to abstract it? Would we even want the<br>
> > > > > >> same name as "normal" slists?<br>
> > > > > >><br>
> > > > > >> (Regardless, I think it would be a great idea for someone to ADT-ify the<br>
> > > > > >> other linked lists!)<br>
> > > > > ><br>
><br>
><br>
><br>
<br>
</div></div></blockquote></div><br></div></div>