[lld] r241420 - COFF: Make ArchiveFile::getMember thread-safe.

[Chandler Carruth]

On Sun, Jul 5, 2015 at 3:52 PM Rui Ueyama <ruiu at google.com> wrote:

> Author: ruiu
> Date: Sun Jul  5 17:50:00 2015
> New Revision: 241420
>
> URL: http://llvm.org/viewvc/llvm-project?rev=241420&view=rev
> Log:
> COFF: Make ArchiveFile::getMember thread-safe.
>
> This function is called SymbolTable::readObjects, so in order to
> parallelize that function, we have to make this function thread-safe.
>

I think it would be better to make an entire interface thread-safe, and
document its thread safety guarantees than to do this one function at a
time.

Also, have you thought about a testing strategy for the concurrency
functionality you're adding? I'm wondering if this would be a place where
unittesting would be effective. There you could spawn a collection of
threads with a lambda to query the thread safe API and ensure they all
produce the same result. When run with ThreadSanitizer, this would provide
a pretty useful way to ensure the APIs that are documented as thread safe
are in fact safe.


> Modified:
>     lld/trunk/COFF/InputFiles.cpp
>
> Modified: lld/trunk/COFF/InputFiles.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/lld/trunk/COFF/InputFiles.cpp?rev=241420&r1=241419&r2=241420&view=diff
>
> ==============================================================================
> --- lld/trunk/COFF/InputFiles.cpp (original)
> +++ lld/trunk/COFF/InputFiles.cpp Sun Jul  5 17:50:00 2015
> @@ -18,6 +18,7 @@
>  #include "llvm/Support/Debug.h"
>  #include "llvm/Support/Endian.h"
>  #include "llvm/Support/raw_ostream.h"
> +#include <mutex>
>
>  using namespace llvm::object;
>  using namespace llvm::support::endian;
> @@ -76,7 +77,9 @@ std::error_code ArchiveFile::parse() {
>  }
>
>  // Returns a buffer pointing to a member file containing a given symbol.
> +// This function is thread-safe.
>  ErrorOr<MemoryBufferRef> ArchiveFile::getMember(const Archive::Symbol
> *Sym) {
> +  static std::mutex Mu;
>

A function-local static mutex is more expensive than necessary.

1) It is a global lock, which seems like it should be avoided.

2) It requires a thread-safe initialization check on each call through the
function in addition to the mutex lock.

It would seem more natural for the ArchiveFile object to carry the mutex
used to establish thread safety for its member functions.


>    auto ItOrErr = Sym->getMember();
>    if (auto EC = ItOrErr.getError())
>      return EC;
> @@ -84,7 +87,9 @@ ErrorOr<MemoryBufferRef> ArchiveFile::ge
>
>    // Return an empty buffer if we have already returned the same buffer.
>    const char *StartAddr = It->getBuffer().data();
> +  Mu.lock();
>    auto Pair = Seen.insert(StartAddr);
> +  Mu.unlock();
>    if (!Pair.second)
>      return MemoryBufferRef();
>    return It->getMemoryBufferRef();
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150705/63176007/attachment.html>