<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Apr 20, 2016 at 3:36 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="">> On 2016-Apr-20, at 14:44, George Burgess IV <<a href="mailto:george.burgess.iv@gmail.com">george.burgess.iv@gmail.com</a>> wrote:<br>
><br>
> I'm probably missing something, but will std::vector move if you mark TrackingMDRef's move ctor noexcept?<br>
><br>
> I ask because throwing `static_assert(std::is_nothrow_move_constructible<llvm::TrackingMDRef>::value, "");` into TrackingMDRef.h fails to compile on my box, unless I explicitly mark TrackingMDRef's move ctor noexcept. IIRC, std::vector has to copy when reallocating unless T's move ctor is noexcept.<br>
<br>
</span>Yes, that's it. Good eye.<br>
<br>
I filed PR27442 on libc++ and then moved it to clang:<br>
<a href="https://llvm.org/bugs/show_bug.cgi?id=27442" rel="noreferrer" target="_blank">https://llvm.org/bugs/show_bug.cgi?id=27442</a><br>
<br>
Does this mean that std::vector pessimizes all of our data structures?<br>
<div><div class="h5"><br>
> On Wed, Apr 20, 2016 at 2:39 PM, Duncan P. N. Exon Smith via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
><br>
> > On 2016-Apr-20, at 14:15, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
> ><br>
> >><br>
> >> On 2016-Apr-20, at 14:12, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
> >><br>
> >>><br>
> >>> On 2016-Apr-20, at 13:59, Duncan P. N. Exon Smith via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
> >>><br>
> >>>><br>
> >>>> On 2016-Apr-20, at 13:28, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> >>>><br>
> >>>><br>
> >>>><br>
> >>>> On Wed, Apr 20, 2016 at 1:14 PM, Duncan P. N. Exon Smith via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
> >>>> Author: dexonsmith<br>
> >>>> Date: Wed Apr 20 15:14:09 2016<br>
> >>>> New Revision: 266909<br>
> >>>><br>
> >>>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=266909&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=266909&view=rev</a><br>
> >>>> Log:<br>
> >>>> IR: Use SmallVector instead of std::vector of TrackingMDRef<br>
> >>>><br>
> >>>> Don't use std::vector<TrackingMDRef>, since (at least in some versions<br>
> >>>> of libc++) std::vector apparently copies values on grow operations<br>
> >>>> instead of moving them. Found this when I was temporarily deleting the<br>
> >>>> copy constructor for TrackingMDRef to investigate a performance<br>
> >>>> bottleneck.<br>
> >>>><br>
> >>>> Got a build log? That seems unlikely, since we have vectors of unique_ptr in the codebase. Perhaps you used some specific operation that required a copy?<br>
> >><br>
> >> It may be that it uses copy if available; otherwise move.<br>
> >><br>
> >>>> I imagine the append you had in the previous patch? Should've been an append with move_iterators? (& maybe that should've been 'assign' instead, I think - in passing)<br>
> >>><br>
> >>> It's the `emplace_back()` calls.<br>
> >><br>
> >> Copied in build log for the one in DIBuilder:<br>
> >><br>
> >>>><br>
> >>>> Modified:<br>
> >>>> llvm/trunk/include/llvm/IR/DIBuilder.h<br>
> >>>> llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp<br>
> >>>> llvm/trunk/lib/IR/DIBuilder.cpp<br>
> >>>><br>
> >>>> Modified: llvm/trunk/include/llvm/IR/DIBuilder.h<br>
> >>>> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/DIBuilder.h?rev=266909&r1=266908&r2=266909&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/DIBuilder.h?rev=266909&r1=266908&r2=266909&view=diff</a><br>
> >>>> ==============================================================================<br>
> >>>> --- llvm/trunk/include/llvm/IR/DIBuilder.h (original)<br>
> >>>> +++ llvm/trunk/include/llvm/IR/DIBuilder.h Wed Apr 20 15:14:09 2016<br>
> >>>> @@ -51,7 +51,11 @@ namespace llvm {<br>
> >>>> bool AllowUnresolvedNodes;<br>
> >>>><br>
> >>>> /// Each subprogram's preserved local variables.<br>
> >>>> - DenseMap<MDNode *, std::vector<TrackingMDNodeRef>> PreservedVariables;<br>
> >>>> + ///<br>
> >>>> + /// Do not use a std::vector. Some versions of libc++ apparently copy<br>
> >>>> + /// instead of move on grow operations, and TrackingMDRef is expensive to<br>
> >>>> + /// copy.<br>
> >>>> + DenseMap<MDNode *, SmallVector<TrackingMDNodeRef, 1>> PreservedVariables;<br>
> >>>><br>
> >>>> DIBuilder(const DIBuilder &) = delete;<br>
> >>>> void operator=(const DIBuilder &) = delete;<br>
> >>>><br>
> >>>> Modified: llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp<br>
> >>>> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp?rev=266909&r1=266908&r2=266909&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp?rev=266909&r1=266908&r2=266909&view=diff</a><br>
> >>>> ==============================================================================<br>
> >>>> --- llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp (original)<br>
> >>>> +++ llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp Wed Apr 20 15:14:09 2016<br>
> >>>> @@ -107,7 +107,12 @@ class BitcodeReaderMetadataList {<br>
> >>>> bool AnyFwdRefs;<br>
> >>>> unsigned MinFwdRef;<br>
> >>>> unsigned MaxFwdRef;<br>
> >>>> - std::vector<TrackingMDRef> MetadataPtrs;<br>
> >>>> +<br>
> >>>> + /// Array of metadata references.<br>
> >>>> + ///<br>
> >>>> + /// Don't use std::vector here. Some versions of libc++ copy (instead of<br>
> >>>> + /// move) on resize, and TrackingMDRef is very expensive to copy.<br>
> >>>> + SmallVector<TrackingMDRef, 1> MetadataPtrs;<br>
> >>>><br>
> >>>> LLVMContext &Context;<br>
> >>>> public:<br>
> >>>><br>
> >>>> Modified: llvm/trunk/lib/IR/DIBuilder.cpp<br>
> >>>> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DIBuilder.cpp?rev=266909&r1=266908&r2=266909&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DIBuilder.cpp?rev=266909&r1=266908&r2=266909&view=diff</a><br>
> >>>> ==============================================================================<br>
> >>>> --- llvm/trunk/lib/IR/DIBuilder.cpp (original)<br>
> >>>> +++ llvm/trunk/lib/IR/DIBuilder.cpp Wed Apr 20 15:14:09 2016<br>
> >>>> @@ -614,7 +614,7 @@ DIGlobalVariable *DIBuilder::createTempG<br>
> >>>><br>
> >>>> static DILocalVariable *createLocalVariable(<br>
> >>>> LLVMContext &VMContext,<br>
> >>>> - DenseMap<MDNode *, std::vector<TrackingMDNodeRef>> &PreservedVariables,<br>
> >>>> + DenseMap<MDNode *, SmallVector<TrackingMDNodeRef, 1>> &PreservedVariables,<br>
> >>>> DIScope *Scope, StringRef Name, unsigned ArgNo, DIFile *File,<br>
> >>>> unsigned LineNo, DIType *Ty, bool AlwaysPreserve, unsigned Flags) {<br>
> >>>> // FIXME: Why getNonCompileUnitScope()?<br>
> >><br>
> >> The code is:<br>
> >> --<br>
> >> PreservedVariables[Fn].emplace_back(Node);<br>
> >> --<br>
> >><br>
> >> Commenting out TrackingMDRef copy constructor and assignment, this compiles with SmallVector.<br>
> ><br>
> ><br>
> > If I *also* comment out the TypedTrackingMDRef copy constructor and assignment, this compiles with std::vector. But it looks to me like std::vector (at least the version I'm using) prefers the copying versions.<br>
><br>
> Hmm. printf is fairly convincing. The compile errors are a bug, but it must be while evaluating some type trait. std::vector is still doing the move that we expect.<br>
<br>
</div></div>... I actually have a fever right now, so I'm probably not to be trusted at all. But printf shouldn't have been convincing, since I only ran it on a tiny IR example. This wasn't just something I fixed by inspection. I actually saw TrackingMDRef copy constructors being called from std::vector<TrackignMDRef>::emplace_back.<br>
<br>
> I'll revert shortly.<br>
<br>
And so I'm not going to revert until we have a new direction that will avoid the copying.<br>
<br>
Options:<br>
<br>
1. Delete TrackingMDRef(const TrackingMDRef&). This would be pretty annoying, but would force people to do the right thing. During graph construction, the copy constructor is expensive and should be used only when necessary. (The move constructor isn't exactly cheap, but it's cheaper.)<br>
<br>
2. Make std::vector on non-trival-copy illegal in our codebase. In that case, my patch was correct.<br>
<br>
3. Add a noexcept specification to TrackingMDRef(TrackingMDRef &&).<br>
<br>
David, you were talking over IRC (before you left) about using SFINAE, but I think we were talking past each other (my fault: I couldn't quite remember the original motivation when you sent your review here...). </blockquote><div><br></div><div>Nah, think that was me - I was rushed/not listening to you properly. Sorry about that.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> My root problem is that std::vector::emplace_back is choosing to copy TrackingMDRef even though it's expensive to do so. </blockquote><div><br></div><div>I think we'd need to understand how that happens at runtime - so far as I understand it should only happen if an exception is thrown and std::vector is trying to rollback the transaction to provide the strong exception guarantee. If no throws happen, I don't think the copy ctor should ever actually be invoked... </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">There's no compile error to SFINAE on. The compile errors only came up when I was investigating the cause of the copies, commenting out code left and right.</blockquote></div><br></div></div>