Skip to content
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

Replace pthread specifics with C11 thread-local variables #811

Merged
merged 2 commits into from
Feb 28, 2018
Merged

Conversation

Adlai-Holler
Copy link
Member

@Adlai-Holler Adlai-Holler commented Feb 26, 2018

C11 thread-local variables are faster and safer than pthread_specifics. Let's use them!

Unfortunately, the pretty alias thread_local is defined in threads.h, an optional part of C11 that Clang doesn't support yet. But the actual _Thread_local storage classifier works just fine. It calls into tlv_get_addr provided by dyld, and it's a lot faster than pthread_getspecific.

Docs: http://en.cppreference.com/w/c/language/storage_duration

Here's the latest source available:
https://opensource.apple.com/source/dyld/dyld-519.2.2/src/threadLocalHelpers.s.auto.html

So 32-bit ARM this is built on pthread_getspecific. 64-bit ARM uses some crazy assembly that I don't understand.

pthread_setspecific(key, NULL);
ASDisplayNodeCAssertNotNil(tls_context, @"Attempt to pop context when there wasn't a context!");
CFRelease((__bridge CFTypeRef)tls_context);
tls_context = nil;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used CFRelease directly here, since we don't actually want to bridge the value into ARC (no need to return id, void is fine.).

@Adlai-Holler
Copy link
Member Author

The danger warning is a false-positive. That file was created under Pinterest, so the current header correctly shows Copyright (c) 2017-present, Pinterest, Inc. cc @garrettmoon is this from your recent change or did I screw it up?

@ghost
Copy link

ghost commented Feb 28, 2018

1 Warning
⚠️ Please ensure license is correct for ASAssert.m:

//
//  ASAssert.m
//  Texture
//
//  Copyright (c) 2014-present, Facebook, Inc.  All rights reserved.
//  This source code is licensed under the BSD-style license found in the
//  LICENSE file in the /ASDK-Licenses directory of this source tree. An additional
//  grant of patent rights can be found in the PATENTS file in the same directory.
//
//  Modifications to this file made after 4/13/2017 are: Copyright (c) through the present,
//  Pinterest, Inc.  Licensed under the Apache License, Version 2.0 (the "License");
//  you may not use this file except in compliance with the License.
//  You may obtain a copy of the License at
//
//      http://www.apache.org/licenses/LICENSE-2.0
//

    

Generated by 🚫 Danger

@garrettmoon
Copy link
Member

@Adlai-Holler I haven't made any changes to the dangerfile except to add an include so that it can read remote files again after the ruby upgrade 😬

@Adlai-Holler Adlai-Holler merged commit a105525 into master Feb 28, 2018
bernieperez pushed a commit to AtomTickets/Texture that referenced this pull request Apr 25, 2018
…up#811)

* Replace pthread specifics with C11 thread-local variables for speed and safety

* Increment changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants