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.

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

Jump to bottom

Document that take uses spinning #20

Closed
tmandry opened this issue Mar 20, 2020 · 2 comments · Fixed by #25
Closed

Document that take uses spinning #20

tmandry opened this issue Mar 20, 2020 · 2 comments · Fixed by #25
Labels
documentation Improvements or additions to documentation

Comments

@tmandry
Copy link

tmandry commented Mar 20, 2020

Spinning has different performance/energy characteristics than mutexes/condition vars, and it would be good to document that this is what take does.
@hawkw
Copy link
Owner

hawkw commented Mar 20, 2020 •
edited

Other options include:

  • Actually using a condvar here.
    • The reason we're not using one now is that it would only be used in take, and would mean adding a mutex/condvar pair to every slot. This would mean two heap allocations per slot, plus increasing the size of each slot itself by 32 bytes (on linux/x86_64, see https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=5c07e4a6cbf9e3755ff5543a9062d6d9)
    • Using a mutex/condvar pair would also be an obstacle to porting this code to no_std, which is an eventual goal...although we could just make the take method require std.
  • Just remove this API entirely in 0.1. I'm not sure how much of a use-case there is for it OTTOMH (tracing-subscriber doesn't use it).

@hawkw hawkw added the documentation Improvements or additions to documentation label Mar 24, 2020
@hawkw
Copy link
Owner

hawkw commented Mar 25, 2020 •
edited

Another option is that we could consider adding a proper backoff here. We would still be spinning, but could have the thread sleep while spinning rather than just emitting pause instructions. That would be an obstacle to porting to no_std, but this code currently doesn't work on no_std, and we could just feature flag it.

This might be a good compromise between not burning CPU in a tight spin loop and having to allocate a mutex/condvar pair for every slot.

hawkw added a commit that referenced this issue Mar 25, 2020
@hawkw
docs(Slab::take): document that take spins
67524dc 
This commit updates the API docs for `Slab::take` to note explicitly
that the calling thread is blocked by spinning until outstanding
references to the removed slot are dropped. The docs now warn against
using `take` if references to a slot may last for a long time, and note
that it should only be used when such references are relatively
short-lived.

Fixes #20

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw mentioned this issue Mar 25, 2020
Merged
@hawkw hawkw closed this as completed in #25 Mar 25, 2020
hawkw added a commit that referenced this issue Mar 25, 2020
@hawkw
docs(Slab::take): document that take spins
447c67b 
This commit updates the API docs for `Slab::take` to note explicitly
that the calling thread is blocked by spinning until outstanding
references to the removed slot are dropped. The docs now warn against
using `take` if references to a slot may last for a long time, and note
that it should only be used when such references are relatively
short-lived.

Fixes #20

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Improve spinning in Slot::take hawkw/sharded-slab
2 participants
@tmandry @hawkw