rust-lang / libs-team

The home of the library team
Apache License 2.0
116 stars 18 forks source link

Implement `From<&'a &'static str>` for `Arguments<'a>` #257

Open EFanZh opened 1 year ago

EFanZh commented 1 year ago

Proposal

Problem statement

Implement From<&'a &'static str> for Arguments<'a>.

Motivating examples or use cases

The log! macro from the log crate once had an optimization for calls with a string literal, but later the optimization was removed to support capturing variables in log messages (see rust-lang/log#446).

The idea is that constructing an Arguments object is more costly than constructing an &str object, so in order to reduce the binary size, the log! macro could dispatch the call to two different functions based on whether the message is a string literal:

fn __private_api_log_1(args: &'static str, ...) { ... }
fn __private_api_log_2(args: Arguments, ...) { ... }

macro_rules! log {
    // If the argument is a string literal,
    // call `__private_api_log_1`,
    // otherwise call `__private_api_log_2`.
}

I intend to bring that optimization back, the problem is that although we can construct an Arguments object from an &str object using format_args!("{s}"), the result Arguments object won’t has its as_str method returning Some(&'static str), which essentially prevents logging backend from utilizing as_str.

Solution sketch

The standard library provides a way to construct an Arguments object that returns Some(&'static str) if called on its as_str() method from an &'static str object. I think implementing From<&'a &'static str> for Arguments<'a> seems to be a reasonable thing to do this.

Alternatives

The only thing I think of is utilizing private API Arguments::new_const, but it’s not a good idea.

Links and related work

Implementation PR: rust-lang/rust#114531

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

Second, if there's a concrete solution:

m-ou-se commented 1 year ago

I'd go further and go for a conversion or constructor from &'static str, without the extra reference. That's impossible with the current implementation of fmt::Arguments, but with some tweaks we could make that work. I think it's worth supporting that.

(I haven't made any progress on https://github.com/rust-lang/rust/issues/99012 lately, but I will look into making conversion from &'static str possible when I find some time to work on fmt::Arguments, hopefully soon.)

m-ou-se commented 1 year ago

I will look into making conversion from &'static str possible when I find some time to work on fmt::Arguments, hopefully soon.

Did exactly that in https://github.com/rust-lang/rust/pull/115129

I'm still tweaking things a bit for performance, but it's looking good. :)

m-ou-se commented 1 year ago

The idea is that constructing an Arguments object is more costly than constructing an &str object

Ideally, that wouldn't be the case. I think we can, eventually, make fmt::Arguments and &str the same size (two pointers/usizes). (The highest bit of the string length is never set, because slices have a maximum length of isize::MAX. So, we can use one bit to differentiate between the trivial case (just a &'static str) and the case with placeholders and arguments. But that's still something that needs experimentation.)

Zoxc commented 1 year ago

Both this API and Arguments::as_str are unreasonable restrictive and I haven't yet seen a reason to have them. There's an assumption that &str is fundamentally better for code size, which is not true.

m-ou-se commented 12 months ago

@Zoxc What do you mean? You don't see a reason for these APIs to exist, or you don't see a reason for certain restrictions? Which restrictions do you mean?

Zoxc commented 11 months ago

I mean the APIs place restrictions on the implementation of Arguments for no reason.

This proposal seems want to pass a &str instead of Arguments, but that's exactly the reason Arguments::as_str was added. There's no new API needed for that.

The existence for Arguments::as_str is without (presented) motivation as there's no reason to favor &str over Arguments to reduce code size if Arguments is further optimized. This function wouldn't be as bad if it could always return None, but currently it requires a pointer to a constant str to be accessible from Arguments.