Skip to content

parallel: add support to explicitly destroy TLS keys #98

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.

Sign up for GitHub

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

Open
compnerd wants to merge 1 commit into
base: main
Choose a base branch
from
Open

parallel: add support to explicitly destroy TLS keys #98

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion Sources/PenguinParallel/ConcurrencyPlatform.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,8 @@ public protocol RawThreadLocalStorage {

/// Makes a new key; the returned key should be used for the entire process lifetime.
static func makeKey() -> Key
/// Invalidates a previously constructed key, freeing resources
static func destroyKey(_ key: inout Key)
Copy link
Owner

@saeta saeta Jul 16, 2020

Choose a reason for hiding this comment

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

Would a better design be for the Key's destructor to handle cleanup?

Copy link
Collaborator Author

@compnerd compnerd Jul 17, 2020

Choose a reason for hiding this comment

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

The destroyKey is different from the key destructor - it frees the TLS slot for re-use, it does not handle the cleanup of the TLS key itself, which remains the duty of the TLS destructor.

/// Retrieves the raw pointer associated with the given key.
static func get(for key: Key) -> UnsafeMutableRawPointer?
/// Sets the raw pointer associated with the given key.
Expand All @@ -158,7 +160,7 @@ public struct TypedThreadLocalStorage<Underlying: RawThreadLocalStorage> {

/// Token used to index into the thread local storage.
public struct Key<Value: AnyObject> {
fileprivate let key: Underlying.Key
fileprivate var key: Underlying.Key
Copy link
Owner

@saeta saeta Jul 16, 2020

Choose a reason for hiding this comment

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

Note: if Underlying.Key is a class, then I think this change can be reverted. WDYT?

Copy link
Collaborator Author

@compnerd compnerd Jul 17, 2020

Choose a reason for hiding this comment

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

We can revert this change even without that - I call that out in the commit message with an explanation of why I changed this. It's purely for detection purposes. If we say that the user is responsible for knowing when they have invalidated the TLS slot, this can remain a let binding. One could argue that the destroy is an explicit action - most usage will be for the lifetime of the program, in which case, you just leak the slot which is irrelevant because the application is terminating and the resources will be released as a byproduct. But, if you want to change the underlying Key to class even outside of that reasoning, I can certainly do that.


/// The thread-local value associated with `self`.
public var localValue: Value? {
Expand All @@ -176,6 +178,11 @@ public struct TypedThreadLocalStorage<Underlying: RawThreadLocalStorage> {
Key(key: Underlying.makeKey())
}

/// Deallocates a key for type `T`.
Copy link
Owner

@saeta saeta Jul 16, 2020

Choose a reason for hiding this comment

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

Ditto.

public static func destroyKey<T: AnyObject>(_ key: inout Key<T>) {
Underlying.destroyKey(&key.key)
}

/// Retrieves the thread-local value for `key`, if it exists.
public static func get<T: AnyObject>(_ key: Key<T>) -> T? {
guard let ptr = Underlying.get(for: key.key) else { return nil }
Expand Down
9 changes: 9 additions & 0 deletions Sources/PenguinParallel/ThreadLocalStorage.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,15 @@ public struct PosixThreadLocalStorage: RawThreadLocalStorage {
Key()
}

public static func destroyKey(_ key: inout Key) {
#if os(Windows)
fatalError("Unimplemented!")
#else
pthread_key_delete(key.value)
Copy link
Owner

@saeta saeta Jul 16, 2020

Choose a reason for hiding this comment

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

Let's change Key to a class, and move this into deinit. WDYT?

key.value = 0
#endif
}

public static func get(for key: Key) -> UnsafeMutableRawPointer? {
pthread_getspecific(key.value)
}
Expand Down