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

std: Modernize the local_data api #13835

Merged
merged 1 commit into from
May 8, 2014
Merged

Conversation

alexcrichton
Copy link
Member

This commit brings the local_data api up to modern rust standards with a few key
improvements:

  • All functionality is now exposed as a method on the keys themselves. Instead
    of importing std::local_data, you now use "key.set()" and "key.get()".
  • All closures have been removed in favor of RAII functionality. This means that
    get() and get_mut() no long require closures, but rather return
    Option where the smart pointer takes care of relinquishing the
    borrow and also implements the necessary Deref traits
  • The modify() function was removed to cut the local_data interface down to its
    bare essentials (similarly to how RefCell removed set/get).

[breaking-change]

@lifthrasiir
Copy link
Contributor

This change would also mean that the "key" suffix commonly attached to the TLS data is no longer appropriate.

@bill-myers
Copy link
Contributor

Is there a reason to keep get_mut and borrow tracking in there, as opposed to only having a get() and having the user use RefCell or Cell if they want mutability?

I think the code (which seems to have semantics equivalent to RefCell) was originally written before RefCell was introduced, and a change like this would be the time to revisit this.

EDIT: hmm, thinking more about it, the naive implementation doesn't seem quite possible due to key insertion and removal; but maybe it can at least use RefCell internally?

@alexcrichton
Copy link
Member Author

That's an interesting idea! That makes sense to me as the best course of action to take.

let curspot = map.iter().position(|entry| {
/// Retrieves a mutable value from TLS. The closure provided is yielded
/// `Some` of a reference to the mutable value located in TLS if one exists,
/// or `None` if the key provided is not present in TLS currently.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is out of date.

@bill-myers
Copy link
Contributor

So, looking more deeply at this, the key insight is that the code boxes all values in ~T as ~LocalData: this seems to be a fundamental requirement, both due to destruction and due to the fact that otherwise enlarging the map would invalidate all borrowed pointers, resulting in memory unsafety.

So what could be done is replacing the ~T with an Rc and thus have an API providing the ability to get/set the Rc (or possibly a Weak, using a WeakKey key type).

Internally, it should be stored as Rc; that's impossible without DST, but it should be possible to get equivalent results manually by storing a transmuted Rc to uint plus a bare function that destroys it.

The user can then use RefCell for mutability; using RefCell internally instead just wastes memory because you can no longer combine the refcnt with the borrow flags, so it's actually not such a good idea.

With this design, one could also trivially use pthread_getspecific/pthread_setspecific with native tasks, by passing the transmuted Rc there, and the destructor bare function to pthread_key_create.

I'm not quite sure if this would be an improvement though, because it uses more memory, due to the refcnt and borrow flags being split (in case the user uses RefCell), and Rc also having a weak count (but with the advantage that TLS lookup and borrowing is decoupled, and that you can have weak references to the TLS boxed data).

At any rate, there should probably be a separate key type that just provides get/set of an uint, without the need of any extra allocation, reference counting or borrowing.

A key type providing get/set of an Option<~T>, setting it to None on get, is also a possibility.

@alexcrichton
Copy link
Member Author

This PR is focused on getting the API right. There are many shortcomings of the implementation, primarily being that a lookup is a linear scan in a task-local array. I don't think that micro-optimizing this implementation is going to be worth it unless it comes close to native TLS.

I'm still musing over whether to expose mutability as part of the API. So long as borrows are supported, it seems to make sense to go ahead and expose mutable borrows. If you've got the flag you may as well use it.

@bill-myers
Copy link
Contributor

Even more general and simpler solution: provide two key types, parameterized by T, with the constraint that sizeof(T) <= sizeof(uint) (which I think unfortunately requires adding a builtin kind to the compiler to express).

The first key type requires Clone, clones on get and provides a set, and would typically be used with Rc, Rc<Cell> or Rc<RefCell> or with uint; the second key type only provides a swap operation instead of get, and would typically be used with Option<~T> (and in fact might be omitted, since it's of limited use).

The values would be internally transmuted to uint and stored along with a bare function destructor either in an ad-hoc structure or passed to pthread_setspecific or similar native APIs (the size restriction allows to do the latter, as well as storing it unboxed in a map).

EDIT: unfortunately the Clone-based key is unsound in general, because clone() may remove the TLS key; hence, the class needs to be private, and only specializations for Rc, Arc and other "well-behaved" types, plus a version for generic T: Copy should be exposed publicly (and eventually an effect system could be added, allowing to require clone to be free of side effects and eliminate the hack).

@alexcrichton
Copy link
Member Author

I have pushed a change which combines pop and set into one method, replace, and the get_mut method was also removed.

@seanmonstar
Copy link
Contributor

Bump?

@pcwalton
Copy link
Contributor

pcwalton commented May 7, 2014

🤘

This commit brings the local_data api up to modern rust standards with a few key
improvements:

* The `pop` and `set` methods have been combined into one method, `replace`

* The `get_mut` method has been removed. All interior mutability should be done
  through `RefCell`.

* All functionality is now exposed as a method on the keys themselves. Instead
  of importing std::local_data, you now use "key.replace()" and "key.get()".

* All closures have been removed in favor of RAII functionality. This means that
  get() and get_mut() no long require closures, but rather return
  Option<SmartPointer> where the smart pointer takes care of relinquishing the
  borrow and also implements the necessary Deref traits

* The modify() function was removed to cut the local_data interface down to its
  bare essentials (similarly to how RefCell removed set/get).

[breaking-change]
bors added a commit that referenced this pull request May 8, 2014
This commit brings the local_data api up to modern rust standards with a few key
improvements:

* All functionality is now exposed as a method on the keys themselves. Instead
  of importing std::local_data, you now use "key.set()" and "key.get()".

* All closures have been removed in favor of RAII functionality. This means that
  get() and get_mut() no long require closures, but rather return
  Option<SmartPointer> where the smart pointer takes care of relinquishing the
  borrow and also implements the necessary Deref traits

* The modify() function was removed to cut the local_data interface down to its
  bare essentials (similarly to how RefCell removed set/get).

[breaking-change]
@bors bors closed this May 8, 2014
@bors bors merged commit ab92ea5 into rust-lang:master May 8, 2014
@alexcrichton alexcrichton deleted the localdata branch May 11, 2014 18:21
arcnmx pushed a commit to arcnmx/rust that referenced this pull request Jan 9, 2023
Inline all format arguments where possible

This makes code more readale and concise,
moving all format arguments like `format!("{}", foo)` into the more compact `format!("{foo}")` form.

The change was automatically created with, so there are far less change of an accidental typo.

```
cargo clippy --fix -- -A clippy::all -W clippy::uninlined_format_args
```
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.

7 participants