[lld] r248109 - COFF: Run InputFile::parse() in background using std::async().

David Blaikie via llvm-commits llvm-commits at lists.llvm.org
Mon Sep 21 09:59:17 PDT 2015


On Sun, Sep 20, 2015 at 8:56 AM, Rui Ueyama <ruiu at google.com> wrote:

> On Sat, Sep 19, 2015 at 8:28 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> On Sat, Sep 19, 2015 at 8:11 PM, Rui Ueyama via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> Author: ruiu
>>> Date: Sat Sep 19 22:11:16 2015
>>> New Revision: 248109
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=248109&view=rev
>>> Log:
>>> COFF: Run InputFile::parse() in background using std::async().
>>>
>>> Previously, InputFile::parse() was run in batch. We construct a list
>>> of all input files and call parse() on each file using parallel_for_each.
>>> That means we cannot start parsing files until we get a complete list
>>> of input files, although InputFile::parse() is safe to call from
>>> anywhere.
>>>
>>
>> Do the following tasks (the loops over ArchiveQueue and ObjectQueue) need
>> to happen in the order of the queue contents? Or would it be correct to
>> handle these out of order (I think you mentioned concurrently doing this
>> work is currently out of the question - the concurrent symbol table is more
>> complexity than is worthwhile/you're investing in right now, yes?)?
>>
>
> I guess they are safe to be reordered, but I'm not 100% sure. This is the
> place we add symbols to the symbol table for each file -- if all symbols
> are distinctive, reordering the files should be totally safe. If there's a
> conflict, there are many possible combinations.
>
> {Undefined, Undefined} -- One of the two would be inserted. Because
> undefined symbols have no identity, it's safe whichever we choose.
> {Undefined, Defined} -- The defined symbol replaces the undefined.
> {Undefined, Lazy} -- The file for the lazy symbol will be parsed.
> {Lazy, Defined} -- The defined overwrites the lazy.
> {Defined, Defined} -- Link failure (unless they are comdats).
>
> All but {Lazy, Defined} seems to be okay to happen in any order. Not sure
> about {Lazy, Defined}. My intuition is that that's also safe but because it
> involves huge side effects, I don't know the answer.
>

Fair, sounds about right/likely to be OK.

the other thing to worry about would be stability of diagnostic output -
might have to cache it all and sort it or something.


>
> If they can be correctly handled out of order you might get some more
>> efficiency by not blocking on the first future, but looking for the next
>> available future (so you can start doing work on the first available one
>> even if others are still being parsed - in the worst case if the first
>> element is the largest/longest to parse, you might be single-threaded for
>> some time as all the other parsing is finished and you're just waiting for
>> the last one)
>>
>> I seem to recall Chandler bemoaning the absence of a "give me the next
>> available future from a colleciton of futures" in the standard library, so
>> this would probably involve substantially more work (& again, may not be
>> the right time/effort/benefit tradeoff right now) to write such tools - if
>> my memory serves me correctly/this hasn't been fixed in the intervening
>> years since I remember having that conversation.
>>
>
> I think we want here is a concurrent queue rather than a function that
> aggregates multiple futures. If we have such data structure, we can let
> each worker to send a InputFile* to the queue when it finishes parsing a
> file. Because we don't have a thread pool nor a concurrent queue, I didn't
> choose to do that, but it would be probably better than this code because
> it wouldn't use futures at all.
>

Fair enough, makes sense.

- Dave


>
>
>> - Dave
>>
>>
>>>
>>> This patch makes it asynchronous. As soon as we add a file to the symbol
>>> table, we now start parsing the file using std::async().
>>>
>>> This change shortens self-hosting time (650 ms) by 28 ms. It's about 4%
>>> improvement.
>>>
>>> Modified:
>>>     lld/trunk/COFF/SymbolTable.cpp
>>>     lld/trunk/COFF/SymbolTable.h
>>>
>>> Modified: lld/trunk/COFF/SymbolTable.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/lld/trunk/COFF/SymbolTable.cpp?rev=248109&r1=248108&r2=248109&view=diff
>>>
>>> ==============================================================================
>>> --- lld/trunk/COFF/SymbolTable.cpp (original)
>>> +++ lld/trunk/COFF/SymbolTable.cpp Sat Sep 19 22:11:16 2015
>>> @@ -27,10 +27,12 @@ void SymbolTable::addFile(std::unique_pt
>>>    InputFile *File = FileP.get();
>>>    Files.push_back(std::move(FileP));
>>>    if (auto *F = dyn_cast<ArchiveFile>(File)) {
>>> -    ArchiveQueue.push_back(F);
>>> +    ArchiveQueue.push_back(
>>> +        std::async(std::launch::async, [=]() { F->parse(); return F;
>>> }));
>>>      return;
>>>    }
>>> -  ObjectQueue.push_back(File);
>>> +  ObjectQueue.push_back(
>>> +      std::async(std::launch::async, [=]() { File->parse(); return
>>> File; }));
>>>    if (auto *F = dyn_cast<ObjectFile>(File)) {
>>>      ObjectFiles.push_back(F);
>>>    } else if (auto *F = dyn_cast<BitcodeFile>(File)) {
>>> @@ -56,13 +58,11 @@ void SymbolTable::readArchives() {
>>>    if (ArchiveQueue.empty())
>>>      return;
>>>
>>> -  parallel_for_each(ArchiveQueue.begin(), ArchiveQueue.end(),
>>> -                    [](ArchiveFile *File) { File->parse(); });
>>> -
>>>    // Add lazy symbols to the symbol table. Lazy symbols that conflict
>>>    // with existing undefined symbols are accumulated in LazySyms.
>>>    std::vector<Symbol *> LazySyms;
>>> -  for (ArchiveFile *File : ArchiveQueue) {
>>> +  for (std::future<ArchiveFile *> &Future : ArchiveQueue) {
>>> +    ArchiveFile *File = Future.get();
>>>      if (Config->Verbose)
>>>        llvm::outs() << "Reading " << File->getShortName() << "\n";
>>>      for (Lazy &Sym : File->getLazySymbols())
>>> @@ -82,25 +82,21 @@ void SymbolTable::readObjects() {
>>>
>>>    // Add defined and undefined symbols to the symbol table.
>>>    std::vector<StringRef> Directives;
>>> -  for (size_t I = 0; I < ObjectQueue.size();) {
>>> -    parallel_for_each(ObjectQueue.begin() + I, ObjectQueue.end(),
>>> -                      [](InputFile *File) { File->parse(); });
>>> -    for (size_t E = ObjectQueue.size(); I != E; ++I) {
>>> -      InputFile *File = ObjectQueue[I];
>>> +  for (size_t I = 0; I < ObjectQueue.size(); ++I) {
>>> +    InputFile *File = ObjectQueue[I].get();
>>> +    if (Config->Verbose)
>>> +      llvm::outs() << "Reading " << File->getShortName() << "\n";
>>> +    // Adding symbols may add more files to ObjectQueue
>>> +    // (but not to ArchiveQueue).
>>> +    for (SymbolBody *Sym : File->getSymbols())
>>> +      if (Sym->isExternal())
>>> +        addSymbol(Sym);
>>> +    StringRef S = File->getDirectives();
>>> +    if (!S.empty()) {
>>> +      Directives.push_back(S);
>>>        if (Config->Verbose)
>>> -        llvm::outs() << "Reading " << File->getShortName() << "\n";
>>> -      // Adding symbols may add more files to ObjectQueue
>>> -      // (but not to ArchiveQueue).
>>> -      for (SymbolBody *Sym : File->getSymbols())
>>> -        if (Sym->isExternal())
>>> -          addSymbol(Sym);
>>> -      StringRef S = File->getDirectives();
>>> -      if (!S.empty()) {
>>> -        Directives.push_back(S);
>>> -        if (Config->Verbose)
>>> -          llvm::outs() << "Directives: " << File->getShortName()
>>> -                       << ": " << S << "\n";
>>> -      }
>>> +        llvm::outs() << "Directives: " << File->getShortName()
>>> +                     << ": " << S << "\n";
>>>      }
>>>    }
>>>    ObjectQueue.clear();
>>>
>>> Modified: lld/trunk/COFF/SymbolTable.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/lld/trunk/COFF/SymbolTable.h?rev=248109&r1=248108&r2=248109&view=diff
>>>
>>> ==============================================================================
>>> --- lld/trunk/COFF/SymbolTable.h (original)
>>> +++ lld/trunk/COFF/SymbolTable.h Sat Sep 19 22:11:16 2015
>>> @@ -16,6 +16,13 @@
>>>  #include "llvm/Support/Allocator.h"
>>>  #include "llvm/Support/raw_ostream.h"
>>>
>>> +#ifdef _MSC_VER
>>> +// <future> depends on <eh.h> for __uncaught_exception.
>>> +#include <eh.h>
>>> +#endif
>>> +
>>> +#include <future>
>>> +
>>>  namespace llvm {
>>>  struct LTOCodeGenerator;
>>>  }
>>> @@ -104,8 +111,8 @@ private:
>>>    llvm::DenseMap<StringRef, Symbol *> Symtab;
>>>
>>>    std::vector<std::unique_ptr<InputFile>> Files;
>>> -  std::vector<ArchiveFile *> ArchiveQueue;
>>> -  std::vector<InputFile *> ObjectQueue;
>>> +  std::vector<std::future<ArchiveFile *>> ArchiveQueue;
>>> +  std::vector<std::future<InputFile *>> ObjectQueue;
>>>
>>>    std::vector<BitcodeFile *> BitcodeFiles;
>>>    std::vector<SmallVector<char, 0>> Objs;
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150921/a775fb93/attachment.html>


More information about the llvm-commits mailing list