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

Use concurrent DispatchQueue #2

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ryannair05
Copy link

Allows multiple read operations to happen in parallel. Write operations stay thread-safe with barrier flags.

Allows multiple read operations to happen in parallel. Write operations stay thread-safe with barrier flags.
@christianselig
Copy link
Owner

christianselig commented Oct 9, 2024

Great idea, thank you so much for the PR! I'm dumb and pushed something up without checking PRs, is there any way you could incorporate the new changes into this PR then I'm happy to merge it?

Also, more of a question than anything, what's the advantage/point of capturing self explicitly in the barrier blocks?

@christianselig
Copy link
Owner

christianselig commented Oct 10, 2024

Actually I gave a quick swing at bringing in the changes (I now understand why you use [self], haha), and upon trying to compile it under Swift 6 there's a bunch of @​Sendable errors that I'm not sure how to fix so I might have to defer to someone who knows more about Sendable integration

@kylebrowning
Copy link

kylebrowning commented Oct 10, 2024

@christianselig got a branch? I can take a swing. Semi. unrelated, is there a reason everything is in one file, would be nice to break things out.

@christianselig
Copy link
Owner

@kylebrowning I mostly just took the code from this PR that created the concurrent queue and changed the write calls to async barriers, should be pretty quick! It was mostly around UserDefaults in the migrate function and the optional any Codable in the bulkStore function

For one file, good question! Just felt like a single file utility mostly, my appetite starts to wane when it gets closer to the 1000 line mark. How were you thinking of splitting it up?

@@ -179,7 +179,7 @@ public final class TinyStorage: @unchecked Sendable {
/// 4. This `migrate` function does not support nested collections due to Swift not having any `AnyCodable` type and the complication in supporting deeply nested types. That means `[String: Any]` is fine, provided `Any` is not another array or dictionary. The same applies to Arrays, `[String]` is okay but `[[String]]` is not. This includes arrays of dictionaries. This does not mean `TinyStorage` itself does not support nested collections (it does), however the migrator does not. You are still free to migrate these types manually as a result (in which case look at the `bulkStore` function).
/// 5. As TinyStorage does not support mixed collection types, neither does this `migrate` function. For instance an array of `[Any]` where `Any` could be a `String` or `Int` is invalid, as is `[String: Any]` where `Any` is not one consistent type.
public func migrate(userDefaults: UserDefaults, keys: Set<String>, overwriteIfConflict: Bool) {
dispatchQueue.sync {
dispatchQueue.async(flags: .barrier) { [self] in

Choose a reason for hiding this comment

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

Not sure if this will be possible while also building for Swift 6, as UserDefaults does not conform to Sendable

I read a semi-related discussion here:
https://forums.developer.apple.com/forums/thread/757527

@bradwindy
Copy link

Another suggested code change I have to solve the other Swift 6 error would be to make TinyStorageBulkStoreItem and its value property conform to Sendable

public struct TinyStorageBulkStoreItem: Sendable {
    let key: any TinyStorageKey
    let value: any Codable & Sendable
    
    public init(key: any TinyStorageKey, value: any Codable & Sendable) {
        self.key = key
        self.value = value
    }
}

@christianselig
Copy link
Owner

@bradwindy I nixed TinyStorageBulkStoreItem recently so maybe that helps?

@jasonnerothin
Copy link

JM2c: I am targeting iOS18 and swift language level 5.

It might be easy enough to just build with these settings and adopt Swift 6 as a next step?

@christianselig
Copy link
Owner

JM2c: I am targeting iOS18 and swift language level 5.

It might be easy enough to just build with these settings and adopt Swift 6 as a next step?

With Swift 6 now being the "standard", I'd prefer to keep the library as in-step with it as possible if that makes sense :(

@ryannair05
Copy link
Author

Instead of having the compiler not check the whole class for Sendable errors in Swift 6, I modified it so only the dictionaryRepresentation and source don't get checked while everything else still does

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

Successfully merging this pull request may close these issues.

5 participants