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

Rework Async tree-sitter Model, Fix Strong Ref Cycle #225

Merged
merged 9 commits into from
Feb 1, 2024

Conversation

thecoolwinter
Copy link
Collaborator

@thecoolwinter thecoolwinter commented Jan 22, 2024

Description

  • Reworks the asynchronous tree-sitter client to use a serial dispatch queue rather than an amalgamation of DispatchQueue and Swift Tasks. Improving code readability and safety.
  • Updates SwiftTreeSitter to 0.8.0, incorporating API changes and improvements from that package.
  • Fixes a strong reference cycle in the SwiftUI wrapper that caused editors to stay in memory after the view was released as well as the Highlighter class.
  • Does some clean up to the highlighter object and a few other spelling errors around the project.
  • Reworks the asynchronous tree-sitter model to correctly use Swift's structured concurrency.

In more detail:
The current async tree-sitter implementation uses a mix of Swift Tasks, pthread locks and a custom task scheduler to perform highlight operations asynchronously. This is dangerous for a variety of reasons, so the TreeSitterClient object has been reworked.

Instead of a class, it is now an actor. Meaning it's still a reference type, but now all asynchronous accesses are performed serially using swift's process pool. This removes the need for any custom locking mechanism, and reduces the likelihood of data races and deadlocks. This also means all operations on the client must be performed asynchronously, so the HighlightProviding protocol has been updated to use async functions in place of callbacks. Finally, the Highlighter class has been updated to keep a set of currently running tasks and provides a method to cancel them if needed.

The client has been reworked to use a single DispatchQueue to coordinate serial access to the client. This allows the object to perform an operation either synchronously or asynchronously depending on a variety of factors. In addition to dispatching all operations to a serial queue, we enforce usage of the client from the main queue to ensure all method calls are performed serially.

I did mention in my comment that I wanted to use locks, but ended up not needing any due to the use of serial queues.

This change is an improvement for two reasons:

  • Increased clarity and safety due to not misusing Swift concurrency features. Plain GCD calls are well-known and predictable and code is much more readable (imho).
  • We can decide to perform operations synchronously if it's safe. For instance, if we wanted to create an auto-completion filter that uses tree-sittter we can attempt to use the tree-sitter client synchronously in a Filter object rather than being forced to muck around with Tasks and async calls. In addition the client will simply throw an error if a synchronous call is unsafe. It'll be up to the caller to decide if an asynchronous call should be performed afterwards.

The strong reference cycle occurred in the Coordinator for CodeEditSourceEditor, which was keeping an optional (but not weak) reference to the text view controller. This has been made weak, correctly referencing the controller. There was also a strong reference cycle between Highlighter and the text view controller that has been corrected.

Related Issues

  • N/A

Checklist

  • I read and understood the contributing guide as well as the code of conduct
  • The issues this PR addresses are related to each other
  • My changes generate no new warnings
  • My code builds and runs on my machine
  • My changes are all related to the related issue above
  • I documented my code

@thecoolwinter thecoolwinter marked this pull request as draft January 26, 2024 05:02
@thecoolwinter
Copy link
Collaborator Author

thecoolwinter commented Jan 26, 2024

Actually, I'd like to rethink using actors here. I'm going to mark this as a draft, and open PRs for the few small bugs fixed in this one.

My thinking is:

  • Actors simplify the syntax for async code, but leave us with a severe limitation. That limitation is the inability to run synchronously when it's okay to do so. For instance, if we are dealing with a small document.
  • Asynchronous, ordered, operations could be performed on a dispatch queue rather than using an actor. This would allow for an API that can be attempted to run synchronously, and otherwise dispatched to a background queue.

The first point is a serious limitation for things like filters, that need to run synchronously. For instance if we wanted an HTML completion filter that fills in tags using tree-sitter information. That filter needs a synchronous API and tree-sitter is fast enough to perform the necessary queries in time for an operation like that. But, if an actor is used we cannot perform that operation synchronously.

Instead, the tree-sitter operation should be attempted synchronously. If it is determined it may take too long to perform on the main thread an error is thrown and the API caller must either re-call the API using the async version or return.

The API would look like:

// Somewhere that needs tree sitter information
let client = TreeSitterClient(...)

do {
    try client.withLayerSync(...) { [weak self] in
        // use captured language layer
    }
} catch {
    if let error = error as? TreeSitterClientError {
        switch error {
            case .notSyncSafe: ...
                // retry with async call
                client.getLayerAsync { [weak self] in
                    // use captured language layer.
                }
        }
    }
}

This will require a locking mechanism that will be handled by the client, but because this system wouldn't use Swift's concurrency features like before I feel much safer using OS locks.

If anyone has some thoughts please post them here!

@thecoolwinter thecoolwinter marked this pull request as ready for review January 28, 2024 21:42
@thecoolwinter thecoolwinter merged commit b5a8ca9 into CodeEditApp:main Feb 1, 2024
2 checks passed
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.

2 participants