<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Oct 3, 2013 at 4:00 PM, Manman Ren <span dir="ltr"><<a href="mailto:manman.ren@gmail.com" target="_blank">manman.ren@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote"><div><div class="h5">On Thu, Oct 3, 2013 at 1:48 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">
<div><div>On Thu, Oct 3, 2013 at 1:33 PM, Manman Ren <span dir="ltr"><<a href="mailto:manman.ren@gmail.com" target="_blank">manman.ren@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">
<div>On Thu, Oct 3, 2013 at 11:38 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">
<div>On Thu, Oct 3, 2013 at 11:26 AM, Manman Ren <span dir="ltr"><<a href="mailto:manman.ren@gmail.com" target="_blank">manman.ren@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">
<div>On Wed, Oct 2, 2013 at 5:55 PM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div>><br>
>><br>
>> I would like to revisit this (maybe revert this) later on when type units<br>
>> are working.<br>
><br>
><br>
> I'd rather revert this before type units (& then revisit this afterwards) as<br>
> it may complicate the type unit work. I think the extra benefit (using<br>
> direct ref_addr, rather than signatures) of this approach over type units<br>
> may be implementable via an incremental improvement to type units.<br>
><br>
<br>
</div>FWIW I've been following along with the thread and agree here. David<br>
is actively working on type units and we should have something here<br>
shortly. Let's let that work proceed and revert this for now.<br></blockquote><div><br></div></div><div>I think we do agree that this approach has some<span style="font-family:arial,sans-serif;font-size:13px"> extra benefit (using direct ref_addr, rather than signatures) over type units.</span></div>
<div><font face="arial, sans-serif">The type-unit work can achieve what this approach does in removing DIE duplication, but using signatures rather than ref_addr.</font></div><div><font face="arial, sans-serif">It may be possible to get the extra benefit via an incremental improvement to type units, but we are not certain about that.</font></div>
</div></div></div></blockquote><div><br></div></div><div>Agreed.</div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote">
<div><font face="arial, sans-serif"><br></font></div><div><font face="arial, sans-serif">I also want to emphasize this patch after wrapping up CU::getDIE as David sugggested is not big, the list of changes can be found at the end of this email.</font></div>
</div></div></div></blockquote><div><br></div></div><div>I don't quite understand this. The list of changes looks incomplete (the insert*DIE functions in DwarfDebug and the maps they use, etc, are not shown here) - are you describing this as "changes" as distinct from "additions"? Even adding completely new code comes at a maintenance burden that should be weighed against the value added.<br>
</div></div></div></div></blockquote><div><br></div></div><div>Oops, I forgot to add the change to the header file: a single map added to DwarfDebug and the corresponding access functions. (5 lines of code)</div><div>
<div><br></div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>
<br>& this doesn't look like how I would expect the code to look after my suggestion (for one thing, I think I was suggesting having one map in DwarfDebug, rather than one per kind of thing - and simply having getDIE (and insertDIE, for that matter) delegate based on a list of known tag types that should be cross-CU versus CU-local)</div>
</div></div></div></blockquote><div><br></div></div><div>Yes, that is what I am going to implement, the list I gave earlier is based on the current design, but the number of changes will be the same.</div><div>Change to header file: 5 lines of code</div>
<div>Change to CU::getDIE to delegate tags to DwarfDebug::getDIE: 5 lines of code</div><div>The changes list in my earlier message: 20 lines of code</div><div>I guess for type units, we need to wrap up addDIEEntry to use ref_signature, so part of the change can be reused by type units.</div>
</div></div></div></blockquote><div><br></div></div></div><div>I think it might be easier to discuss this in terms of reverting your current patch and then looking at the complete new patch we're discussing/proposing.</div>
</div></div></div></blockquote><div><br></div></div></div><div>I am not proposing a complete new patch, I am going to change the current patch:</div></div></div></div></blockquote><div><br></div><div>Yeah - I just think it'll be easier to discuss as completely separate patch, on top of Clang before r191792 (or, actually, with r191792 reverted), rather than as a mutation to your existing patch.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>1> combine 3 maps into a single map</div>
<div>2> wrap up CU::getDIE so we don't need to modify from the existing CU::getDIE to DwarfDebug::getDIE</div>
<div>These should not affect functionality.</div><div class="im"><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>I don't quite get the complication caused by this approach to type units. When we start constructing a DIE for a type MDNode, we either use the type unit or we use<br></div>
<div>the standard approach of creating a DIE, my changes touch the standard approach a little by moving the map from CU to DwarfDebug.</div></div></div></div></blockquote><div><br></div></div><div>Yep, it might come out this cleanly, though I'm a little cautious about putting all these things in together then trying to clean it up/evaluate the changes appropriately.</div>
<div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>When referring to the type, we either use ref_signature or ref_addr or ref4.</div>
<div><br></div><div>Even after type units start working, people can still choose to use either type units or ref_addr|ref4 for a period of time before their tool chains are updated to support type units.</div><div>As for the tool chains, extra time may be required to set up a map from the type signature to the actual address in the Dwarf.</div>
</div></div></div></blockquote><div><br></div></div><div>Certainly - as I said, I don't doubt your change has some value over type units. There's a question as to how much value and how best to fit it in, if we do, to the existing code.</div>
<div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>The performance improvement of this patch is kind of straight-forward, if we have X CUs refer to the same type MDNode, X copies of type DIEs will be created<br></div><div>without this patch. </div></div></div></div>
</blockquote><div><br></div></div><div>Yes, I agree that it has value.</div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote"><div>I do have some numbers collected before, I will dig that out.</div></div></div></div></blockquote><div><br></div></div><div>Thanks, that'd be very helpful.<br></div></div></div></div></blockquote>
</div><div>3GB memory reduction for xalan built with lto -g (without removing DIE duplication, the memory usage is 7GB)</div></div></div></div></blockquote><div><br></div></div><div>OK, so you're looking at memory usage during LTO, not the size of the resulting debug info? (that's fine, just trying to understand exactly what metrics you're targeting)<br>
<br>& this is peak total memory usage (how/what exactly are you measuring?) during LTO?</div></div></div></div></blockquote></div><div>Yes, the peak total memory usage when building xalan, for the raw DWAF file size, it reduces the raw DWARF debug info from 58M to 7M.</div>
</div></div></div></blockquote><div><br></div><div>Neat. (thanks for the numbers)<br><br>Does Clang self-host with LTO at the moment? I realize it's probably a painfully big target to try to run, and perhaps without further improvement it won't be achievable on reasonable hardware due to memory issues, but I figured I'd ask because it's a nice common baseline that all the Clang/LLVM developers can reproduce.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div class="im">
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">
<div class="gmail_extra"><div class="gmail_quote"><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br>(but I'm also particularly interested in how much value this has over type units - which is one reason I'd like to do this after type units, either that or we'll need to add flags or somesuch to disable this optimization so we can compare type units with and without this change)<br>
</div></div></div></div></blockquote></div><div>The value will be the advantage of ref_addr over ref_signature after type units are fully working.</div></div></div></div></blockquote><div><br></div></div><div>Yes, I understand the mechanical benefit, when I speak of this I mean looking at the actual numbers. The % benefit in whatever metrics we care about.</div>
<div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>If I revert this change, we have a choice of modifying the standard route of ref4 to support ref_addr, as it is currently implemented;<br>
</div></div></div></div></blockquote><div><br></div></div><div>I don't quite understand what you're saying here. </div></div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div>
<div>or implementing this on top of type units (but we are not certain that we can implement this via an incremental improvement to type units, and what benefit did we gain compared</div><div>to modifying the standard route).</div>
</div></div></div></blockquote><div><br></div></div><div>I'm not necessarily suggesting that this feature would be implemented in a highly interrelated way with type units, but that seeing how it layers on top of type units may be useful.</div>
</div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>But it provides us huge memory reduction before type units are full working and tool chains are updated.<br></div></div></div></div></blockquote><div><br></div></div><div>Tool chain support is a fair point, if lldb doesn't and won't support type units any time soon, then the value of this feature over today's behavior (or tomorrow's behavior with type units disabled, etc) is a relevant metric to consider.<br>
<br>I'm still inclined to prefer backing this patch out and discussing the right design rather than trying to coerce this into the right design from where it is now. It'll make the patch history and impact clearer.</div>
</div></div></div></blockquote><div><br></div></div><div>Since we are already in discussion about this, let's reach a conclusion about what the right design for this feature is and what is the relationship of this feature (remove duplicated DIEs for a single MDNode and use ref_addr)</div>
<div>to the type units stuff.</div><div><br></div><div>If we are going to take a similar approach as this commit, I don't need to wait for the type units stuff to be working. I can revert the current patch and combine it with the changes suggested above into a single patch.</div>
<div><div>1> combine 3 maps into a single map</div><div>2> wrap up CU::getDIE so we don't need to modify from the existing CU::getDIE to DwarfDebug::getDIE</div></div><div><br></div><div>I don't think it is worthwhile to wait for the type units working and see how this feature layers on top of type units.</div>
</div></div></div></blockquote><div><br></div><div>I'm not entirely convinced of this, but I'm willing to discuss the design of this feature in parallel as I work on type units - I don't want to hold this patch back just to put type units in first for the sake of it, but I'm still concerned about the interaction of these two patches (which is usually a case of "whoever gets in first gets in first and the other guy can deal with resolving the differences" - which I'm fine with, once we've discussed and come to an appropriate conclusion on the design).</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>I see this as complementary to the type units, especially if we want to support this feature with type units disabled.<br></div><div>Do you have any suggestion on how to layer this feature on top of type units?</div>
</div></div></div></blockquote><div><br></div><div>My thinking around layering this on top of type units was to move the callback into DwarfDebug a little higher. In my prototype patch, CompileUnit constructs the DIE for the type, then passes it up to DwarfDebug to finish or reject - if DwarfDebug decides to create a type unit, it builds the type unit, attaches the hash to the type, and returns "accepted" to CompileUnit, which stops there, doing nothing more to the type DIE. If DwarfDebug rejects the type (not creating a type unit), CompileUnit continues on, building the full type description.<br>
<br>For cross-CU cached types, essentially we'd just want to move the type construction up a layer - rather than CompileUnit constructing an empty DIE, we'd call DwarfDebug which would return us a DIE fully constructed, or nothing. If DwarfDebug returned nothing, CompileUnit would construct a type unit as normal.<br>
<br>The registration side of things might be more work (how does DwarfDebug learn about existing types) - one way might be for DwarfDebug to always own calling back into the CompileUnit (either the existing/current CU, or the Type Unit it just constructed) to finish the work, then registering the newly created type before returning back to CompileUnit.<br>
<br>This wouldn't solve the registration of functions and static member DIEs in the referencing CUs.</div><div><br></div><div><br></div><div>I'm not sure how well that'll work. The alternative, which is more in line with what we've already been discussing, as you say, may be orthogonal/compatible with type units. Simply having "getDIE" query the tag type and for some specific set of tags, delegate to DwarfDebug. Have "insertDIE" do the same thing (factor out the test for the specific kind of tag to a common function) and go with that. <br>
<br>I haven't quite thought about the work list aspect of your patch yet. I'll have to give that some consideration.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><br></div><div>As to the complication this can cause type units, I think we have some agreement:</div><div class="im"><div><div style="font-family:arial,sans-serif;font-size:13px"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I don't quite get the complication caused by this approach to type units. When we start constructing a DIE for a type MDNode, we either use the type unit or we use<br>
</div><div>the standard approach of creating a DIE, my changes touch the standard approach a little by moving the map from CU to DwarfDebug.</div></div></div></div></blockquote><div><br></div></div><div style="font-family:arial,sans-serif;font-size:13px">
Yep, it might come out this cleanly, though I'm a little cautious about putting all these things in together then trying to clean it up/evaluate the changes appropriately.</div></div><div style="font-family:arial,sans-serif;font-size:13px">
<br></div></div><span class="HOEnZb"><font color="#888888"><div>Manman</div></font></span><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><span><font color="#888888">
<div><br></div><div>- David</div></font></span><div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><font color="#888888"><div>
<br></div><div>Manman</div></font></span><div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>
<br>The other problem is that removing these patches will be somewhat more difficult after I've spent the next couple of weeks cleaning code up and adding in type units. I think there's enough design discussion to have with regards to how best to implement your feature that I'd rather have that design discussion on top of type units rather than the other way around - though it's possible we can come to a reasonable/good design in either order.</div>
<div><div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><br></div><div><font face="arial, sans-serif">Thanks,</font></div><div><font face="arial, sans-serif">Manman</font></div><div><br></div><div> 15 - insertDIE(Ty, TyDIE);</div><div> 16 + DD->insertTypeDIE(Ty, TyDIE);</div>
<div><br></div><div> 31 - insertDIE(SP, SPDie);</div><div> 32 + DD->insertSPDIE(SP, SPDie);</div><div><br></div><div> 40 - insertDIE(DT, StaticMemberDIE);</div><div> 41 + DD->insertStaticMemberDIE(DT, StaticMemberDIE);</div>
<div><br></div><div> 52 + // Process the worklist to add attributes with the correct form (ref_addr or</div><div> 53 + // ref4).</div><div> 54 + for (unsigned I = 0, E = DIEEntryWorklist.size(); I < E; I++) {</div>
<div>
55 + addDIEEntry(DIEEntryWorklist[I].Die, DIEEntryWorklist[I].Attribute,</div><div> 56 + dwarf::DW_FORM_ref4, DIEEntryWorklist[I].Entry);</div><div> 57 + assert(E == DIEEntryWorklist.size() &&</div>
<div> 58 + "We should not add to the worklist during finalization.");</div><div> 59 + }</div><div> 60 +</div><div><br></div><div> 68 +</div><div> 69 +/// When we don't know whether the correct form is ref4 or ref_addr, we create</div>
<div> 70 +/// a worklist item and insert it to DIEEntryWorklist.</div><div> 71 +void DwarfDebug::addDIEEntry(DIE *Die, uint16_t Attribute, uint16_t Form,</div><div> 72 + DIEEntry *Entry) {</div>
<div> 73 + /// Early exit when we only have a single CU.</div><div> 74 + if (GlobalCUIndexCount == 1 || Form != dwarf::DW_FORM_ref4) {</div><div> 75 + Die->addValue(Attribute, Form, Entry);</div><div> 76 + return;</div>
<div> 77 + }</div><div> 78 + DIE *DieCU = Die->checkCompileUnit();</div><div> 79 + DIE *EntryCU = Entry->getEntry()->checkCompileUnit();</div><div> 80 + if (!DieCU || !EntryCU) {</div><div> 81 + // Die or Entry is not added to an owner yet.</div>
<div> 82 + insertDIEEntryWorklist(Die, Attribute, Entry);</div><div> 83 + return;</div><div> 84 + }</div><div> 85 + Die->addValue(Attribute,</div><div> 86 + EntryCU == DieCU ? dwarf::DW_FORM_ref4 : dwarf::DW_FORM_ref_addr,</div>
<div> 87 + Entry);</div><div> 88 +}</div><div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
(and no, computing the hash isn't enough to matter time wise on anything)<br>
<span><font color="#888888"><br>
-eric<br>
</font></span></blockquote></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>