Skip to content

Rename ThreadPool::as_worker() to with_worker()#15

Merged
NthTensor merged 2 commits intoNthTensor:mainfrom
BD103:with-worker
Jun 19, 2025
Merged

Rename ThreadPool::as_worker() to with_worker()#15
NthTensor merged 2 commits intoNthTensor:mainfrom
BD103:with-worker

Conversation

@BD103
Copy link
Copy Markdown
Contributor

@BD103 BD103 commented Jun 16, 2025

The docs of ThreadPool::as_worker() describe it as a potentially expensive operation. However, the as_ prefix usually signals a cheap and quick operation (see the Rust API Guidelines and examples such as str::as_bytes() and pointer::as_ref()). This can be a problem, as someone reviewing code may assume as_worker() is a cheap operation and not realize it could cause a mutex lock.

This PR renames as_worker() to with_worker(), in a similar vein to LocalKey::with() and the old TaskPool::with_local_executor(). Not only does the with_ prefix signal that it will pass a worker to a closure, but it also doesn't come with any assumptions on its performance.

This is a breaking change, although I'm not sure how you want to document it.

@NthTensor
Copy link
Copy Markdown
Owner

Sweet, thank you so much! I agree, this name is not ideal. I'm still getting the project online a bit, so this will be blocked until I fix CI.

Copy link
Copy Markdown
Owner

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

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

CI is now fixed, but I've made some changes to the tests so you will have to rebase.

@BD103
Copy link
Copy Markdown
Contributor Author

BD103 commented Jun 19, 2025

Ok, it should be all good now! When fixing the conflicts, I ran into this comment:

Forte/src/thread_pool.rs

Lines 633 to 637 in ae7aa8f

/// Workers are the recommended way to interface with a thread pool. To get
/// access to worker for a given thread pool, users should call
/// [`ThreadPool::as_worker`] (which keeps work in the same thread, but
/// sometimes may fail to acquire a worker) or [`ThreadPool::as_worker`] (which
/// may send work to other threads, but will always acquire a worker).

Specifically, this bit:

To get access to worker for a given thread pool, users should call ThreadPool::as_worker (which keeps work in the same thread, but sometimes may fail to acquire a worker) or ThreadPool::as_worker (which may send work to other threads, but will always acquire a worker).

I combined it into:

To get access to worker for a given thread pool, users should call ThreadPool::with_worker.

@NthTensor
Copy link
Copy Markdown
Owner

Yep, artifact of an older design. I need to do a pass on the docs, there's a few places where I may have left outdated stuff. Thanks for catching that one.

@NthTensor NthTensor merged commit ee18adc into NthTensor:main Jun 19, 2025
4 checks passed
@BD103 BD103 deleted the with-worker branch June 19, 2025 14:11
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