[PATCH] D38431: [ProfileData] Fix data racing in merging indexed profiles

[Rong Xu via llvm-commits]

On Fri, Sep 29, 2017 at 4:32 PM, Vedant Kumar via Phabricator <
reviews at reviews.llvm.org> wrote:

> vsk added inline comments.
>
>
> ================
> Comment at: lib/ProfileData/InstrProfReader.cpp:736
>  Error IndexedInstrProfReader::readNextRecord(NamedInstrProfRecord
> &Record) {
> -  static unsigned RecordIndex = 0;
> +  thread_local static unsigned RecordIndex = 0;
>
> ----------------
> Why isn't this just a member variable?
>
> If we used "thread_local", wouldn't there still be a bug if you
> instantiated and used two IndexedInstrProfReaders?
>

I think making this member variable would also work -- and this might be a
better way.

But why you think "thread_local" won't
work? IndexedInstrProfReader::readNextRecord can not be multithreaded in
either way. If we instantiate two IndexedInstrProfReaders, each should be
in it own thread and accesses its own RecordIndex. There is not racing.


>
>
> ================
> Comment at: tools/llvm-profdata/llvm-profdata.cpp:230
>      // Load the inputs in parallel (N/NumThreads serial steps).
> -    unsigned Ctx = 0;
> -    for (const auto &Input : Inputs) {
> -      Pool.async(loadInput, Input, Contexts[Ctx].get());
> -      Ctx = (Ctx + 1) % NumThreads;
> +    unsigned ChunkSize = Inputs.size() / NumThreads;
> +    unsigned ChunkRemainder = Inputs.size() % NumThreads;
> ----------------
> I don't understand the problem here. Each time a WriterContext is
> accessed, the accessing thread immediately acquires a unique lock on the
> context. Where does the racing access occur?
>

Where the lock to WriterContext comes from? I only see the lock for the
queue in ThreadPool. I thought it was the user's responsibility to make
sure the share data is race free.


>
>
> https://reviews.llvm.org/D38431
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171002/82471fa9/attachment.html>