<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Oct 2, 2017, at 11:40 AM, Rong Xu <<a href="mailto:xur@google.com" class="">xur@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><br class="Apple-interchange-newline"><br style="font-family: Helvetica; font-size: 14px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="gmail_quote" style="font-family: Helvetica; font-size: 14px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">On Mon, Oct 2, 2017 at 11:20 AM, Vedant Kumar<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:vsk@apple.com" target="_blank" class="">vsk@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><br class=""><div class=""><span class=""><blockquote type="cite" class=""><div class="">On Oct 2, 2017, at 10:58 AM, Rong Xu <<a href="mailto:xur@google.com" target="_blank" class="">xur@google.com</a>> wrote:</div><br class="m_-2794571362470032571Apple-interchange-newline"><div class=""><div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Fri, Sep 29, 2017 at 4:32 PM, Vedant Kumar via Phabricator<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:reviews@reviews.llvm.org" target="_blank" class="">reviews@reviews.llvm.org</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;">vsk added inline comments.<br class=""><br class=""><br class="">================<br class="">Comment at: lib/ProfileData/InstrProfReade<wbr class="">r.cpp:736<br class=""> Error IndexedInstrProfReader::readNe<wbr class="">xtRecord(NamedInstrProfRecord &Record) {<br class="">- static unsigned RecordIndex = 0;<br class="">+ thread_local static unsigned RecordIndex = 0;<br class=""><br class="">----------------<br class="">Why isn't this just a member variable?<br class=""><br class="">If we used "thread_local", wouldn't there still be a bug if you instantiated and used two IndexedInstrProfReaders?<br class=""></blockquote><div class=""><br class=""></div><div class="">I think making this member variable would also work -- and this might be a better way.</div></div></div></div></div></blockquote><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><br class=""></div><div class="">But why you think "thread_local" won't work? IndexedInstrProfReader::<wbr class="">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.</div></div></div></div></div></blockquote><div class=""><br class=""></div></span><div class="">I was concerned about the case where two IndexedInstrProfReaders are used by the same thread. Or two IndexedInstrProfReaders are created, but are used intermittently by two different threads. Admittedly we're not doing these things now, but because the API permits this type of usage, it needs to work properly.</div><span class=""><br class=""></span></div></div></blockquote><div class="">Yes. If two Readers are created by the same thread, then 'thread_local" will break. But at the same time, if two threads are access the one Reader, member variable solution will break too. API does not mention any of the permitted usage as of now. Maybe we should document it.</div></div></div></blockquote><div><br class=""></div><div>Presumably if two threads were to access a single reader, they would use some form of synchronization. I think that this requirement is clear from context, and don't see a pressing need to document it.</div><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family: Helvetica; font-size: 14px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class=""><span class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><br class=""><br class="">================<br class="">Comment at: tools/llvm-profdata/llvm-profd<wbr class="">ata.cpp:230<br class=""> // Load the inputs in parallel (N/NumThreads serial steps).<br class="">- unsigned Ctx = 0;<br class="">- for (const auto &Input : Inputs) {<br class="">- Pool.async(loadInput, Input, Contexts[Ctx].get());<br class="">- Ctx = (Ctx + 1) % NumThreads;<br class="">+ unsigned ChunkSize = Inputs.size() / NumThreads;<br class="">+ unsigned ChunkRemainder = Inputs.size() % NumThreads;<br class="">----------------<br class="">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?<br class=""></blockquote><div class=""><br class=""></div><div class="">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.</div></div></div></div></div></blockquote></span><span class=""><div class=""><span class=""><br class=""></span></div>Sorry, I see now that locking the WriterContext isn't relevant to this problem.</span></div><div class=""><br class=""></div><div class="">I think I understand where you're coming from now. Your solution would get rid of the race, but I don't think users of IndexedInstrProfReader should have to take care to only use one instance per thread. If we can make "RecordIndex" a member variable, we wouldn't have to change how the API is used, so I'd prefer that solution.</div></div></blockquote><div class=""><br class=""></div><div class="">Sorry I don't follow here. I was talking the race to WriterContext. It was not clear to me that there is lock to WriterContext. Can you explain if there is one?</div></div></div></blockquote><div><br class=""></div><div>The first step in loadInput() is to take a unique lock on the WriterContext. So a thread executing loadInput() has exclusive access to that WriterContext, and exclusive access to the IndexedInstrProfReader constructed later on in the function. See:</div><div><br class=""></div><span class=""><br class="">/// Load an input into a writer context.<br class="">133<span class="Apple-tab-span" style="white-space:pre"> </span>133<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>static void loadInput(const WeightedFile &Input, WriterContext *WC) {<br class="">134<span class="Apple-tab-span" style="white-space:pre"> </span>134<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span> std::unique_lock<std::mutex> CtxGuard{WC->Lock}; // Lock on the writer context is held exclusively after this point.<br class="">135<span class="Apple-tab-span" style="white-space:pre"> </span>135<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><br class="">136<span class="Apple-tab-span" style="white-space:pre"> </span>136<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span> // If there's a pending hard error, don't do more work.<br class="">137<span class="Apple-tab-span" style="white-space:pre"> </span>137<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span> if (WC->Err)<br class="">138<span class="Apple-tab-span" style="white-space:pre"> </span>138<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span> return;<br class="">139<span class="Apple-tab-span" style="white-space:pre"> </span>139<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><br class="">140<span class="Apple-tab-span" style="white-space:pre"> </span>140<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span> WC->ErrWhence = Input.Filename;<br class="">141<span class="Apple-tab-span" style="white-space:pre"> </span>141<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><br class="">142<span class="Apple-tab-span" style="white-space:pre"> </span>142<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span> auto ReaderOrErr = InstrProfReader::create(Input.Filename);<br class=""></span></div><span class=""><br class="">Still, I don't think having a lock on the WriterContext is fundamentally important to this problem. I think it should be safe to create and use an IndexedInstrProfReader if the reader is owned exclusively and not shared between threads.<br class=""><br class="">vedant<br class=""><br class=""><blockquote type="cite" class=""><br class="">Thanks,<br class=""><br class="">-Rong<br class=""> <br class=""><br class="">vedant<br class=""><br class=""><blockquote type="cite" class=""> <br class=""><br class=""><br class=""><a href="https://reviews.llvm.org/D3843" class="">https://reviews.llvm.org/D3843</a>1<br class=""></blockquote></blockquote></span><br class=""></body></html>