Page MenuHomePhabricator

Kill the LLVM global lock
ClosedPublic

Authored by zturner on Jun 13 2014, 4:25 PM.

Details

Summary

This patch kills the LLVM global lock. This was being used in only two places, neither of which required exclusivity from each other, so they have been independently changed to use their own separate mutexes.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 10410.Jun 13 2014, 4:25 PM
zturner retitled this revision from to Change the LLVM global lock to use RAII semantics.
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added a reviewer: chandlerc.
zturner set the repository for this revision to rL LLVM.
zturner added a subscriber: Unknown Object (MLST).
majnemer added inline comments.
include/llvm/Support/Threading.h
21 ↗(On Diff #10410)

The & should bind to the function name, not the return type.

Gah, again. I need to start using clang-format. Thanks for catching this.

zturner updated this revision to Diff 10411.Jun 13 2014, 5:01 PM

Fix the binding of the reference operator.

chandlerc added inline comments.Jun 13 2014, 5:10 PM
lib/Support/Threading.cpp
25 ↗(On Diff #10411)

Is the constructor for a mutex threadsafe? Because we use VS versions which don't provide a thread safe initialization. Actually, even if the constructor is threadsafe, won't that still form a data race if we call this from two threads when built with MSVC?

I think you'll need this to be a static global ultimately... =/

zturner added inline comments.Jun 13 2014, 5:17 PM
lib/Support/Threading.cpp
25 ↗(On Diff #10411)

I should probably put a comment here, but the problem with that is that static constructors can trigger instantiation of ManagedStatics. In this particular CL, that will be fine, because when a ManagedStatic is instantiated during a static constructor, llvm_is_multithreaded() will be false.

But after the rest goes through, multi-threading will always be on, and if the mutex is a global static we won't be able to guarantee that the mutex's constructor will have been called. By definining it in the function, we can enforce the ordering.

By the same logic though, since we call this from static constructors, we know that it will be initialized in a thread-safe fashion. But this is only by chance (since we know we use static constructors), so perhaps we should leave a comment indicating this so that if / when we get entirely rid of static constructors, we will know to go back and fix this.

chandlerc added inline comments.Jun 13 2014, 5:36 PM
lib/Support/Threading.cpp
25 ↗(On Diff #10411)

I don't think that we can reasonably require that this public routine only be called from static constructors and thus already be serialized. Instead, we need to make this reliable in some way. I suspect that the right way is through a global pointer to a mutex and a pthread_once (or analogous) call to a function which allocates the mutex on the heap and sets the pointer to it. We can then "leak" the mutex freely because it isn't actually leaked at all and there will be only one...

But I feel like we're spending too much time on an abstraction that has no utility.

We can remove the usage of this from Timer.cpp by re-using the existing ManagedStatic-based mutex that file already has.

Now we just need ManagedStatic to bootstrap itself a mutex to use, and everything else can rely on it. My suggestion is to sink the pthread_once-style allocation of a single mutex completely into the ManagedStatic code. If necessary, we can even have a Once.inc in Unix/ and Windows/, but I see no reason to really surface the API to other users.

What I wouldn't give for thread-safe function statics...

zturner updated this revision to Diff 10451.Jun 16 2014, 10:52 AM
zturner retitled this revision from Change the LLVM global lock to use RAII semantics to Kill the LLVM global lock.
zturner updated this object.
zturner set the repository for this revision to rL LLVM.

Remove the global lock entirely, and bootstrap a call_once initialized' mutex in ManagedStatic that is safe for use during static construction.

chandlerc edited edge metadata.Jun 17 2014, 9:22 AM

This patch (attached to the email, and in Phab) still contains all the
changes to stop using multithreading as a runtime parameter?

zturner updated this revision to Diff 10508.Jun 17 2014, 10:43 AM
zturner edited edge metadata.
zturner set the repository for this revision to rL LLVM.

Fourth time's a charm.

This patch splits the CL out so that the only thing it does is kill the global lock.

zturner updated this revision to Diff 10511.Jun 17 2014, 11:10 AM

Fix the comment about static initialization.

rnk accepted this revision.Jun 17 2014, 2:06 PM
rnk added a reviewer: rnk.
rnk added a subscriber: rnk.

lgtm

lib/Support/ManagedStatic.cpp
32 ↗(On Diff #10511)

The new function naming convention is camelCase over StudlyCaps:
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

The confusion is understandable, because the existing code is very inconsistent.

This revision is now accepted and ready to land.Jun 17 2014, 2:06 PM

Thanks! Will wait for Chandler's lgtm as well before I push, since he's
had a bunch of comments on past iterations, want to make sure all of his
concerns are addressed as well.

rnk's recommendation offline is to go ahead and submit this, given that it seems like all of Chandler's concerns have been addressed. At the very least, this should be close enough to addressing all of Chandler's concerns that if there is anything outstanding, we can address it in a follow up.

zturner closed this revision.Jun 19 2014, 9:25 AM
zturner updated this revision to Diff 10638.

Closed by commit rL211277 (authored by @zturner).

FYI, LGTM too.