<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jun 26, 2015 at 1:50 PM, Pete Cooper <span dir="ltr"><<a href="mailto:peter_cooper@apple.com" target="_blank">peter_cooper@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><span class=""><blockquote type="cite"><div>On Jun 26, 2015, at 1:42 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><div dir="ltr">Completely shooting from the hip, we have been seeing some slow-downs over the last week or so & there's at least a vague guess that it could be related to these pervasive data structure changes that have been going in within that window.<br></div></div></blockquote></span>Thats a reasonable guess.  I’d think the same.<span class=""><br><blockquote type="cite"><div><div dir="ltr"><br>Have you been running/seen any compile-time perf changes in that same window?<br></div></div></blockquote></span>I have yeah.  We have the bitcode saved from the linker for verify_uselist_order with and without debug info and before/after optimizations.  I’ve been putting those through either opt/llc and measuring compile time.  It never strayed from noise TBH.</div></div></blockquote><div><br></div><div>What about broader perf numbers? Not seeing any significant fluctuations in the last 7-10 days?<br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><span class=""><blockquote type="cite"><div><div dir="ltr">One of the things I was thinking, if this is an issue, is that we could do something vtable-esque. Since we already have an enum to differentiate the various derived classes - could we have an array of manual vtables, indexed by that enum, and explicitly invoke through that? If the switch stuff is slowing things down - possible that it isn't, and the switch is cheaper than the indirect call, etc.<br></div></div></blockquote></span>I was about to reply ‘surely the optimizer will just make a jump table from the switch which is just as fast’.</div></div></blockquote><div><br></div><div>Could be, I really don't know.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>However, then I thought about what that would look like, and its not quite what you’re suggesting.  The jump table optimization would only jump to a block which then does the call, but the call itself is then another jump.<br></div><div><br></div><div>We couldn’t rely on it 100%, but I wonder if we can teach the optimizer to optimize jump tables to calls where every call has identical argument lists.</div><div><br></div><div>Anyway, it probably wouldn’t help with the performance tracking for me to commit this (assuming someone gives it a LGTM), so i’ll hold off for now.</div></div></blockquote><div><br>Yeah, until you/I/we/someone has actually reliable perf stuff that points to any particular change or changes, I probably wouldn't start shooting in the dark - just figured I'd bring it up in case it's something you're already aware of or, upon looking, would become so.<br><br>- Dave<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><span class="HOEnZb"><font color="#888888"><div><br></div><div>Pete</div></font></span><span class=""><div><blockquote type="cite"><div><div dir="ltr"><br>Just some idle thoughts.</div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jun 16, 2015 at 4:46 PM, Pete Cooper <span dir="ltr"><<a href="mailto:peter_cooper@apple.com" target="_blank">peter_cooper@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Nick<br>
<br>
As you were the last person to attempt this on PR889, I thought i’d see how you like this series of patches.<br>
<br>
I’ve tried to devirtualize a small number of virtual methods at a time, hence the bunch of patches.  Unfortunately the last one is pretty huge as it handles all the places we used to call delete on Value’s and now need to call a destroy method to dispatch to the correct subclass.  I’m not sure there’s a great way to split the last one up.<br>
<br>
David, i’ve CCed you specifically to ask about how safe some of the changes here are.  In particular, the way i’ve changed things is:<br>
- If a subclass is final (and i’ve added final where I can), then I give it its own delete method which just does ‘using User::operator delete’.  Is this reasonable?  Do i need to handle any other kinds of delete like User does?<br>
- If a subclass is not able to be final, then i’ve added a destroy() method and switch on the value kind to then call ‘delete cast<SubClass>(this)’<br>
<br>
Thanks to Duncan for the tip on std::default_delete, i’ve made use of this on various places which had a std::unique_ptr of say Instruction or Value.  The default deleter for those calls destroy() instead of delete.  Deleter’s aren’t needed for final subclasses because I was able to give them a delete method instead.<br>
<br>
I also extended helpers like DeleteContainerPointers to take a std::default_delete so that we can forward to the destroy methods where needed.<br>
<br>
I measured memory usage on the verify_uselist_order test case Duncan has been using.  This contains debug info and is measured with 'llc ~/verify-uselistorder.lto.opt.bc -o /dev/null --filetype=obj -disable-verify’.  Prior to this change we consumed 814MB, and 800MB after.  So almost 2% savings.  This is in line with what I was expecting as the savings is 8 bytes per Value, but due to malloc alignment on Mac OS, we only save that on an average of half of values.<br>
<br>
Finally, this is the order I did all the work locally.  If any of this makes sense to be squashed or split, please let me know.  In the first patch for example I create Constant.def, but I later change that to Value.def.  This one should obviously be fixed to be Value.def from the start.<br>
<br>
Cheers,<br>
Pete<br>
<br>
</blockquote></div><br></div>
</div></blockquote></div><br></span></div></blockquote></div><br></div></div>