[llvm] r266909 - IR: Use SmallVector instead of std::vector of TrackingMDRef

Wed Apr 20 16:20:50 PDT 2016

On Wed, Apr 20, 2016 at 3:36 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

> > On 2016-Apr-20, at 14:44, George Burgess IV <george.burgess.iv at gmail.com>
> wrote:
> >
> > I'm probably missing something, but will std::vector move if you mark
> TrackingMDRef's move ctor noexcept?
> >
> > 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.
>
> Yes, that's it.  Good eye.
>
> I filed PR27442 on libc++ and then moved it to clang:
>   https://llvm.org/bugs/show_bug.cgi?id=27442
>
> Does this mean that std::vector pessimizes all of our data structures?
>
> > On Wed, Apr 20, 2016 at 2:39 PM, Duncan P. N. Exon Smith via
> llvm-commits <llvm-commits at lists.llvm.org> wrote:
> >
> > > On 2016-Apr-20, at 14:15, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> > >
> > >>
> > >> On 2016-Apr-20, at 14:12, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> > >>
> > >>>
> > >>> On 2016-Apr-20, at 13:59, Duncan P. N. Exon Smith via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
> > >>>
> > >>>>
> > >>>> On 2016-Apr-20, at 13:28, David Blaikie <dblaikie at gmail.com> wrote:
> > >>>>
> > >>>>
> > >>>>
> > >>>> On Wed, Apr 20, 2016 at 1:14 PM, Duncan P. N. Exon Smith via
> llvm-commits <llvm-commits at lists.llvm.org> wrote:
> > >>>> Author: dexonsmith
> > >>>> Date: Wed Apr 20 15:14:09 2016
> > >>>> New Revision: 266909
> > >>>>
> > >>>> URL: http://llvm.org/viewvc/llvm-project?rev=266909&view=rev
> > >>>> Log:
> > >>>> IR: Use SmallVector instead of std::vector of TrackingMDRef
> > >>>>
> > >>>> Don't use std::vector<TrackingMDRef>, since (at least in some
> versions
> > >>>> of libc++) std::vector apparently copies values on grow operations
> > >>>> instead of moving them.  Found this when I was temporarily deleting
> the
> > >>>> copy constructor for TrackingMDRef to investigate a performance
> > >>>> bottleneck.
> > >>>>
> > >>>> 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?
> > >>
> > >> It may be that it uses copy if available; otherwise move.
> > >>
> > >>>> 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)
> > >>>
> > >>> It's the `emplace_back()` calls.
> > >>
> > >> Copied in build log for the one in DIBuilder:
> > >>
> > >>>>
> > >>>> Modified:
> > >>>>  llvm/trunk/include/llvm/IR/DIBuilder.h
> > >>>>  llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp
> > >>>>  llvm/trunk/lib/IR/DIBuilder.cpp
> > >>>>
> > >>>> Modified: llvm/trunk/include/llvm/IR/DIBuilder.h
> > >>>> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/DIBuilder.h?rev=266909&r1=266908&r2=266909&view=diff
> > >>>>
> ==============================================================================
> > >>>> --- llvm/trunk/include/llvm/IR/DIBuilder.h (original)
> > >>>> +++ llvm/trunk/include/llvm/IR/DIBuilder.h Wed Apr 20 15:14:09 2016
> > >>>> @@ -51,7 +51,11 @@ namespace llvm {
> > >>>>   bool AllowUnresolvedNodes;
> > >>>>
> > >>>>   /// Each subprogram's preserved local variables.
> > >>>> -    DenseMap<MDNode *, std::vector<TrackingMDNodeRef>>
> PreservedVariables;
> > >>>> +    ///
> > >>>> +    /// Do not use a std::vector.  Some versions of libc++
> apparently copy
> > >>>> +    /// instead of move on grow operations, and TrackingMDRef is
> expensive to
> > >>>> +    /// copy.
> > >>>> +    DenseMap<MDNode *, SmallVector<TrackingMDNodeRef, 1>>
> PreservedVariables;
> > >>>>
> > >>>>   DIBuilder(const DIBuilder &) = delete;
> > >>>>   void operator=(const DIBuilder &) = delete;
> > >>>>
> > >>>> Modified: llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp
> > >>>> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp?rev=266909&r1=266908&r2=266909&view=diff
> > >>>>
> ==============================================================================
> > >>>> --- llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp (original)
> > >>>> +++ llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp Wed Apr 20
> 15:14:09 2016
> > >>>> @@ -107,7 +107,12 @@ class BitcodeReaderMetadataList {
> > >>>> bool AnyFwdRefs;
> > >>>> unsigned MinFwdRef;
> > >>>> unsigned MaxFwdRef;
> > >>>> -  std::vector<TrackingMDRef> MetadataPtrs;
> > >>>> +
> > >>>> +  /// Array of metadata references.
> > >>>> +  ///
> > >>>> +  /// Don't use std::vector here.  Some versions of libc++ copy
> (instead of
> > >>>> +  /// move) on resize, and TrackingMDRef is very expensive to copy.
> > >>>> +  SmallVector<TrackingMDRef, 1> MetadataPtrs;
> > >>>>
> > >>>> LLVMContext &Context;
> > >>>> public:
> > >>>>
> > >>>> Modified: llvm/trunk/lib/IR/DIBuilder.cpp
> > >>>> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DIBuilder.cpp?rev=266909&r1=266908&r2=266909&view=diff
> > >>>>
> ==============================================================================
> > >>>> --- llvm/trunk/lib/IR/DIBuilder.cpp (original)
> > >>>> +++ llvm/trunk/lib/IR/DIBuilder.cpp Wed Apr 20 15:14:09 2016
> > >>>> @@ -614,7 +614,7 @@ DIGlobalVariable *DIBuilder::createTempG
> > >>>>
> > >>>> static DILocalVariable *createLocalVariable(
> > >>>>   LLVMContext &VMContext,
> > >>>> -    DenseMap<MDNode *, std::vector<TrackingMDNodeRef>>
> &PreservedVariables,
> > >>>> +    DenseMap<MDNode *, SmallVector<TrackingMDNodeRef, 1>>
> &PreservedVariables,
> > >>>>   DIScope *Scope, StringRef Name, unsigned ArgNo, DIFile *File,
> > >>>>   unsigned LineNo, DIType *Ty, bool AlwaysPreserve, unsigned Flags)
> {
> > >>>> // FIXME: Why getNonCompileUnitScope()?
> > >>
> > >> The code is:
> > >> --
> > >>   PreservedVariables[Fn].emplace_back(Node);
> > >> --
> > >>
> > >> Commenting out TrackingMDRef copy constructor and assignment, this
> compiles with SmallVector.
> > >
> > >
> > > 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.
> >
> > 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.
>
> ... 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.
>
> > I'll revert shortly.
>
> And so I'm not going to revert until we have a new direction that will
> avoid the copying.
>
> Options:
>
>  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.)
>
>  2. Make std::vector on non-trival-copy illegal in our codebase.  In that
> case, my patch was correct.
>
>  3. Add a noexcept specification to TrackingMDRef(TrackingMDRef &&).
>
> 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...).

Nah, think that was me - I was rushed/not listening to you properly. Sorry
about that.

> My root problem is that std::vector::emplace_back is choosing to copy
> TrackingMDRef even though it's expensive to do so.

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...

> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160420/30cbd5ac/attachment-0001.html>