-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid using global Mutex variables #1252
Avoid using global Mutex variables #1252
Conversation
9a552f4
to
72d9e27
Compare
🚫 CI failed with log |
72d9e27
to
2e60133
Compare
Huh, I was under the impression that these new's shouldn't be executed until the variable is accessed ("magic statics" in C11). Could you post the stack trace where they're running early? |
🚫 CI failed with log |
These are run by dlyd before main(). |
This reverts part of 5c9815f. Using Mutex for global C++ variables causes Texture experiments to be triggered before main(). Our (internal) experiment framework isn't ready that early in the process. We can get rid of StaticMutex once the unfair lock experiment is shipped.
2e60133
to
f4e2860
Compare
🚫 CI failed with log |
Oh, right, magic statics only work for function-local statics not global variables. How about instead, we move the static variables into the functions in which they're used? That'll make them lazily generated. Currently each one is only used in one function. Performance will be better if we use dispatch_once than if we rely on lazy statics, so: - (void)myMethod {
static ASDN::Mutex *cacheMutex;
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
cacheMutex = new ASDN::Mutex();
});
// is faster than
static auto *cacheMutex = new ASDN::Mutex();
// because the latter actually uses a global mutex around all accesses to all function-local statics.
} |
8696222
to
a62ac2e
Compare
@Adlai-Holler Good call. I updated the call sites to use
Ohh, good to know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Thanks for reviewing! |
Cool. We're pretty confident this will have the desired lifecycle ever during app teardown, right? As some of the comments mention, the original solution was put in after some tricky-to-debug crashes on suspend. |
@appleguy The lifecycle was a problem because |
After 5c9815f, some
Mutex
es are used as global C++ variables which are loaded beforemain()
. Since theMutex
constructor checks for unfair lock experiment, it triggers an experiment configuration load, and our app isn't ready to respond that early in the process.