Tracking Issue for const-initialized thread locals · Issue #84223 · rust-lang/ru...

alexcrichton commented on Sep 14

I've requested this be tagged I-nominated for discussion for stabilization, but I figured I'd want to paste some background here as well. My personal main motivation for this is optimizing the pieces of Wasmtime where a host calls into a WebAssembly module. Part of that is frobbing some TLS values as we enter into wasm, and this frobbing is actually marked #[inline(never)] for the poor cross-crate codegen that TLS has otherwise. This outlining plus the extra checks for whether or not TLS is initialized is what I'm trying to optimize.

In benchmarks locally the call overhead of host->wasm decreases from 20ns to 18ns (10% reduction) with the usage of const-initialized-thread-locals (which are also #[inline]'d since they have good codegen). This isn't really gonna make or break anyone's day since it's a pretty small difference, but nevertheless I'd love to get it out of the way so I don't have to worry about it again.

Another data point I see linked here is that rustc is using const-initialized thread locals for some moderate gains across the board.

There was some brief discussion on Zulip about the syntax when I originally asked to stabilize. One thing I'll clarify is that this doesn't literally use const { .. } blocks in the expanded code, the macro is simply recognizing it as input syntax. I believe one gotcha of this is that if you have $e:expr in a macro and in the future $e is something like const { 1 } I don't think that it will use the const-initialized version of thread_local! were you to do something like thread_local!(static X: i32 = $e) since I don't think that rustc splits apart tokens to match sub-patterns in a macro. This will, however, work for literal declarations like thread_local!(static X: i32 = const { 1 });

Overall the biggest thing to decide here I think is the syntax. One alternative is a whole new macro, but I think that it'd be ideal if we could keep it in one macro to not have to worry about multiple. Otherwise though I'm happy to implement whatever syntax.

m-ou-se commented on Oct 6

Discussed in the library api team meeting. We'd like @rust-lang/lang to sign off on this.

nikomatsakis commented on Oct 6

This seems like a fine thing to stabilize; however, just to be clear, is this feature any use without const blocks? I guess that one can still stabilize to other constants (e.g., simple integer values)?

alexcrichton commented on Oct 6

This doesn't actually use const { .. } blocks at the compiler level, it's basically just surface syntax to the macro itself. In that sense this should still be useful even if Rust never gets const { .. } blocks because it would still trigger the specialized implementation for this macro where the initialization expression is a constant known at compile time

cramertj commented on Oct 6

It seems a little funky to me to stabilize a syntax that, at a surface level, matches that of a pre-existing unstable feature (const blocks). Even though it may not require const blocks under the hood, in the long-term I think users would expect that the set of things allowed inside thread_local!(static X: Y = const { <code here> }; would match the rules for what could be present in static X: Y = const { ... };.

That is, if we decided that some surface syntax or semantics should be different inside const blocks, I'd also want it to be different in these not-const-blocks. Are we confident that we'll be able to provide matching semantics in each of these contexts in the future? That is, that the not-const-block in thread_local! doesn't allow anything that a const block wouldn't allow?

nikomatsakis commented on Oct 6

Oh, wait, maybe I misunderstood. I didn't realize that const { ... } was part of the macro, I expected it to accept "any expression". That said, I'll have to review, but I would expect that indeed it should accept the same set of code?

alexcrichton commented on Oct 6

For reference, the actual macro is (trimmed down a bit):

macro_rules! thread_local {
    ($(#[$attr:meta])* $vis:vis static $name:ident: $t:ty = const { $init:expr }) => (
        // ...
    );

    ($(#[$attr:meta])* $vis:vis static $name:ident: $t:ty = $init:expr) => (
        // ...
    );

    // ...
}

The actual expanded code ends up doing:

                    #[thread_local]
                    static mut VAL: $t = $init;

I believe what @cramertj is describing is indeed a risk of using the const { ... } syntax in the macro without literally using it in the expanded code. If for whatever reason the code allowed inside of const { ... } differed from that in { ... } then this macro expansion would not reflect that becaue the expansion doesn't actually use const { .. }.

Personally I'm fine implementing some other syntax if there's too many worries about using the const { ... } syntax. What I would probably suggest is an alternative macro, thread_local_const!, or something like that (although I realize there are downsides to this as well and would welcome other thoughts on alternative syntaxes)

nikomatsakis commented on Oct 12

@rfcbot fcp merge

We discussed this in the @rust-lang/lang meeting today. Consensus was that we should move forward with stabilization here, so I am beginning an FCP.

Here are our notes:

• What is the change?
  • Macro looks for const { expr } and changes the code that gets generated
    • currently this gets generated to static FOO: ... = <expr>
• Why this change?
  • Codegen improvement: the initial value of a thread-local is constant and not re-evaluated per thread
• Expected outcome?
  • We stabilize the macro and eventually move it to use inline constants
• What is the risk?
  • The macro currently accepts const { ... } syntax but desugars to a static
  • If the set of expressions accepted as initial value for a static diverged from set that can appear in const { ... } this would be weird, but we consider this unlikely.
  • Moreover, if that were to occur in the future before const is stabilied, we could add const { ... } into the desugaring and use "allow internal unstable",

rfcbot commented on Oct 12 •

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

• LocalKey type resolved by #84223 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

cramertj commented on Oct 12

Moreover, if that were to occur in the future before const is stabilied, we could add const { ... } into the desugaring and use "allow internal unstable",

I don't understand how this would work. If the set of things allowed in const is limited, it'd be a breaking change to move this macro over to use const after this macro has already been available on stable. I'm happy to land as-is if we think the risk of these sorts of additional limitations is low, though.

yaahc commented on Oct 14 •

* If the set of expressions accepted as initial value for a static diverged from set that can appear in `const { ... }` this would be weird, but we consider this unlikely.

If this happened I believe we could fix the inconsistency at an edition boundary the same way we did for panic!, so I don't think this is a particularly large risk.

m-ou-se commented on Oct 17

@rfcbot concern LocalKey type

A variable defined with thread_local!{} is of the type LocalKey<T>. I'm wondering if we should use a different type for const-initialized thread locals.

Right now, reusing the same type works fine, as it just a wrapped function pointer. But I can imagine that a thread local key without lazy initializer might have a different implementation or even api than the one with. (E.g. THREAD_LOCAL.is_initialized() or THREAD_LOCAL.initialize_with(|| ..).)

BurntSushi commented on Oct 18

@rust-lang/lang

If the set of expressions accepted as initial value for a static diverged from set that can appear in const { ... } this would be weird, but we consider this unlikely.

Moreover, if that were to occur in the future before const is stabilied, we could add const { ... } into the desugaring and use "allow internal unstable",

If this happened in a way where more things were accepted by a static than by const { ... }, wouldn't changing thread_local! to use const { ... } internally be a breaking change?

I wonder if we might indulge briefly in a thought experiment. If we knew that static items and const { ... } items diverged in some way, would we still use the const { ... } syntax here? If not, what would we use instead?

alexcrichton commented on Oct 18

With a concern about reusing LocalKey<T> for the underlying type for this, if it were to switch to something like ConstLocalKey<T> (or w/e name) then it would arguably warrant a different syntax as well along the lines of const_thread_local!(...) (or w/e name) which would help make the point of const { ... } blocks moot.

nbdd0121 commented 23 days ago

I am concerned because this differs from the const block.

#![feature(inline_const)]
#![feature(thread_local_const_init)]

use std::cell::Cell;

static V: u32 = 1;

std::thread_local!{
	// This is accepted, because this isn't actually const.
    static KEY: Cell<u32> = const { Cell::new(V) };

	// This is not, because this is actually const, and const cannot refer to statics.
    static KEY2: Cell<u32> = (const { Cell::new(V) });
}

alexcrichton commented 23 days ago

To what extent are the rules for evaluating const { .. } different or the same as const X: ty = ...;? If they're the same then I think that the const-semantic worries could be pretty easily solved by changing the macro expansion to look like:

const INIT: $t = $init;
#[thread_local]
static mut VAL: $t = INIT;

There is still the more fundamental concern though about perhaps not wanting to use LocalKey at all.

alexcrichton commented 10 days ago

I've opened #90774 with my above suggestion.

@m-ou-se do you still believe that this should expand to a different type? Personally I would push back saying that with using the same macro name I would expect that the same kind of LocalKey<T> is generated. If in the future a LocalKeyConstInit<T> or w/e is added then I think that would probably fit best behind a new macro name such as thread_local_const_init!(...) or etc.

If consensus though is that this needs a new type name I can try to implement that too.

m-ou-se commented 4 days ago

@rfcbot resolve LocalKey type

A thread_local_const_init or something like that would probably not only require the initialization expression to be const, but also forbid the type from needing Drop. If the type needs drop, it'll need to keep track of its initialization state anyway.

The only thing to note is that if we do ever add a .is_initialized() or anything similar to LocalKey, the current optimization might no longer work, since it'll need to track the 'initialization' state even when it doesn't need drop.

rfcbot commented 4 days ago

This is now entering its final comment period, as per the review above.

veber-alex commented 2 days ago •

If this is a pure performance optimization, can the macro just "do the right thing" without any user facing syntax?
It will require some compiler magic but maybe it's worth it in this case?
This way also avoids any breaking changes in the future as what is optimized and what isn't is internal and all existing users will benefit without changing code or bumping MSRV.