[PATCH] D34838: Prototype: Reduce llvm-profdata merge memory usage further

[David Blaikie via llvm-commits]

On Thu, Jul 6, 2017 at 11:04 AM David Li via Phabricator <
reviews at reviews.llvm.org> wrote:

> davidxl added a comment.
>
> Limit NamedInstrProfRecord to the reader file seems ok -- but this class
> also looks like a useful convenience class. We can probably discuss that in
> a different thread if it is desirable.
>

Sure


>
>
>
> ================
> Comment at: lib/ProfileData/InstrProfWriter.cpp:180
> +Error InstrProfWriter::addRecord(NamedInstrProfRecord &&I, uint64_t
> Weight) {
> +  // Careful/subtle code: the std::move(I) below doesn't adversely affect
> the
> +  // Name and Hash fields, since the third parameter to addRecord takes
> ----------------
> I don't find the comment here helping.  Perhaps just mention the move
> construction happens later when assigned to the Dest?
>

I refactored the code to hopefully make it obviously correct without the
confusing comment:

  auto Name = I.Name;
  auto Hash = I.Hash;
  func(Name, Hash, std::move(I), ...)

There are about 3 different reasons this code didn't have a potentially
used-after-move bug in it. (one was that it passes by rvalue ref, rather
than by value - so the std::move has no effect at this call site, the other
is that only the base subobject is moved, and the third is that StringRef
and uint64_t are trivially movable (and such a trivial move is actually a
copy, so it leaves the original objects untouched) - but none of that's
really obvious from the code one is looking at right here, so perhaps
writing the defensive/obviously correct code that doesn't rely on any of
those things is better)

- Dave


>
>
> https://reviews.llvm.org/D34838
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170706/27ba8fdb/attachment.html>