Constify {Mutex, Notify, OnceCell, RwLock, Semaphore}::new

[NobodyXu]

Motivation

With MSRV bumped to 1.63, we can now make {Mutex, Notify, OnceCell, RwLock, Semaphore}::new available in const context.

Solution

Removed the tracing log in these functions and makes these function usable in const context when cfg(not(all(loom, test))).

[NobodyXu]

The CI failure is already reported in #5888 and is probably spurious.

[NobodyXu]

This PR needs a restart of the failed CI tests.

[Darksonn]

@hawkw What purpose does the tracing span have? Do we lose something by dropping it like this?

If they do serve a purpose, we could decide to keep the new vs const_new distinction.

[NobodyXu]

Also @carllerche since they are the one who suggested this change.

[Darksonn]

I think its important to ask the question, because once it is const, we can't add back the tracing span.

[NobodyXu]

I think its important to ask the question, because once it is const, we can't add back the tracing span.

I wonder if storing the tracing span is really necessary or have any use.
Who use it now and what is it useful for?

I think it could be only used for tracking where the Semaphore is created and I'm not sure how useful that is.

It also adds quite some space to BatchSemaphore, with tracing enabled each BatchSemaphore is now 4 * pointer + 8 bytes larger than before, on 64-bit platform it is 40 bytes larger.

[Darksonn]

I believe that the spans are used for tokio-console to keep track of which synchronization primitives are used, but I don't know the details.

[inevity]

The pr like task: add Tracing instrumentation to spawned tasks about resource instrument add those code.

[NobodyXu]

The pr like task: add Tracing instrumentation to spawned tasks about resource instrument add those code.

Thanks!
I skimmed through it and it only adds spanning to tasks, not syncrhonisation primitives.

I think we would have to wait for @hawkw to review this PR.

[hds]

While a definitive answer would come from Eliza, I can confirm that as well as tasks, many resources in Tokio are instrumented, including Mutex, RwLock, Semaphore, and BatchSemaphore which are touched in this PR.

For example, for a Semaphore, the instrumentation tracks the number of permits belonging to a semaphore as well as the aquire operations performed by tasks. This instrumentation is also available when Semaphore is used by another sync resource, such as RwLock.

The changes to remove that instrumentation would break support for these resources in tokio-console. I would kindly ask that it isn't removed.

I'm guessing that initializing this instrumentation is not possible in const context and that's why it's being removed. I'm not sure there is currently a way around that.

[hds]

Removing this will break support for tracking batch semaphores in tokio-console.

[hds]

Removing this will break support for tracking mutxes in tokio-console.

[hds]

Removing this will break support for tracking RwLocks in tokio-console.

[hds]

Removing this will break support for tracking RwLocks in tokio-console.

[hds]

Removing this will break support for tracking semaphores in tokio-console.

[NobodyXu]

@hds Thanks for the response, if these spanning is used in tokio-console then removing them would indeed break it.

I'm guessing that initializing this instrumentation is not possible in const context and that's why it's being removed. I'm not sure there is currently a way around that.

I am willing to close this PR since there are already Mutex::const_new for initialization at compile-time, but to be fair this is still a problem for tracing since these Mutex::const_new does not have a span attached.

You could, workaround this, by wrapping the tracing::Span with a OnceCell or Mutex for this to be initialized lazily, but it adds a bit overhead and still would require someone to somehow initializes it (could be done by first call in Mutex::lock).

[Darksonn]

Yeah, lets drop this for now. It's acceptable that const_new doesn't work with tracing — after all, its always been like that.

[NobodyXu]

@hds BTW, since BatchSemaphore already contains a tracing::Span, is it necessary for tokio::sync::{Mutex, Notify, OnceCell, RwLock} to also contain their own tracing::Span, which only adds more overhead?

[hds]

@NobodyXu There are higher level attributes which are captured in the higher level resources. It is true that the performance impact of the tracing feature isn't well quantified, but as it is optional, that is something that is still pending to be measured.

[NobodyXu]

Thanks for the quick response!
Yeah it makes sense to add them then, though its overhead is even larger than I expected.

Problem with this is that after it gets stablised, if one of the dependency enables tracing then everyone of them has to bare with the additional cost.

[hds]

Because of this, I think that the tracing feature will never be able to be stablised in its current form.

It should only be possible for the top level binary to enable tracing. Unfortunately the current tracing feature can't be removed, because that's a breaking change, but it will likely need to be gated by a cfg flag like tokio_unstable as well - even if it gets its own.

[NobodyXu]

Because of this, I think that the tracing feature will never be able to be stablised in its current form.

It should only be possible for the top level binary to enable tracing. Unfortunately the current tracing feature can't be removed, because that's a breaking change, but it will likely need to be gated by a cfg flag like tokio_unstable as well - even if it gets its own.

I think having its own tokio_console_support flag makes sense, otherwise it will be confused as unstable and can be removed at anytime.

Having a tokio_console_support will also make it explicit that it is for tokio_console.