Skip to content

Commit

Permalink
Auto merge of #83302 - camsteffen:write-piece-unchecked, r=dtolnay
Browse files Browse the repository at this point in the history 
Get piece unchecked in `write`

We already use specialized `zip`, but it seems like we can do a little better by not checking `pieces` length at all.

`Arguments` constructors are now unsafe. So the `format_args!` expansion now includes an `unsafe` block.

<details>
<summary>Local Bench Diff</summary>

```text
 name                        before ns/iter  after ns/iter  diff ns/iter   diff %  speedup
 fmt::write_str_macro1       22,967          19,718               -3,249  -14.15%   x 1.16
 fmt::write_str_macro2       35,527          32,654               -2,873   -8.09%   x 1.09
 fmt::write_str_macro_debug  571,953         575,973               4,020    0.70%   x 0.99
 fmt::write_str_ref          9,579           9,459                  -120   -1.25%   x 1.01
 fmt::write_str_value        9,573           9,572                    -1   -0.01%   x 1.00
 fmt::write_u128_max         176             173                      -3   -1.70%   x 1.02
 fmt::write_u128_min         138             134                      -4   -2.90%   x 1.03
 fmt::write_u64_max          139             136                      -3   -2.16%   x 1.02
 fmt::write_u64_min          129             135                       6    4.65%   x 0.96
 fmt::write_vec_macro1       24,401          22,273               -2,128   -8.72%   x 1.10
 fmt::write_vec_macro2       37,096          35,602               -1,494   -4.03%   x 1.04
 fmt::write_vec_macro_debug  588,291         589,575               1,284    0.22%   x 1.00
 fmt::write_vec_ref          9,568           9,732                   164    1.71%   x 0.98
 fmt::write_vec_value        9,516           9,625                   109    1.15%   x 0.99
```
</details>
  • Loading branch information
@bors
bors committed Aug 23, 2021
2 parents a49e38e + b1e07b8 commit de42550
Show file tree
Hide file tree
Showing 14 changed files with 165 additions and 65 deletions.
30 changes: 23 additions & 7 deletions compiler/rustc_builtin_macros/src/format.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ use Position::*;

use rustc_ast as ast;
use rustc_ast::ptr::P;
use rustc_ast::token;
use rustc_ast::tokenstream::TokenStream;
use rustc_ast::{token, BlockCheckMode, UnsafeSource};
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_errors::{pluralize, Applicability, DiagnosticBuilder};
use rustc_expand::base::{self, *};
Expand Down Expand Up @@ -838,12 +838,15 @@ impl<'a, 'b> Context<'a, 'b> {
//
// But the nested match expression is proved to perform not as well
// as series of let's; the first approach does.
let pat = self.ecx.pat_tuple(self.macsp, pats);
let arm = self.ecx.arm(self.macsp, pat, args_array);
let head = self.ecx.expr(self.macsp, ast::ExprKind::Tup(heads));
let result = self.ecx.expr_match(self.macsp, head, vec![arm]);
let args_match = {
let pat = self.ecx.pat_tuple(self.macsp, pats);
let arm = self.ecx.arm(self.macsp, pat, args_array);
let head = self.ecx.expr(self.macsp, ast::ExprKind::Tup(heads));
self.ecx.expr_match(self.macsp, head, vec![arm])
};

let args_slice = self.ecx.expr_addr_of(self.macsp, result);
let ident = Ident::from_str_and_span("args", self.macsp);
let args_slice = self.ecx.expr_ident(self.macsp, ident);

// Now create the fmt::Arguments struct with all our locals we created.
let (fn_name, fn_args) = if self.all_pieces_simple {
Expand All @@ -857,7 +860,20 @@ impl<'a, 'b> Context<'a, 'b> {
};

let path = self.ecx.std_path(&[sym::fmt, sym::Arguments, Symbol::intern(fn_name)]);
self.ecx.expr_call_global(self.macsp, path, fn_args)
let arguments = self.ecx.expr_call_global(self.macsp, path, fn_args);
let body = self.ecx.expr_block(P(ast::Block {
stmts: vec![self.ecx.stmt_expr(arguments)],
id: ast::DUMMY_NODE_ID,
rules: BlockCheckMode::Unsafe(UnsafeSource::CompilerGenerated),
span: self.macsp,
tokens: None,
}));

let ident = Ident::from_str_and_span("args", self.macsp);
let binding_mode = ast::BindingMode::ByRef(ast::Mutability::Not);
let pat = self.ecx.pat_ident_binding_mode(self.macsp, ident, binding_mode);
let arm = self.ecx.arm(self.macsp, pat, body);
self.ecx.expr_match(self.macsp, args_match, vec![arm])
}

fn format_arg(
Expand Down
42 changes: 39 additions & 3 deletions library/core/src/fmt/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

use crate::cell::{Cell, Ref, RefCell, RefMut, UnsafeCell};
use crate::char::EscapeDebugExtArgs;
use crate::iter;
use crate::marker::PhantomData;
use crate::mem;
use crate::num::fmt as numfmt;
Expand Down Expand Up @@ -334,11 +333,29 @@ enum FlagV1 {
impl<'a> Arguments<'a> {
/// When using the format_args!() macro, this function is used to generate the
/// Arguments structure.
#[cfg(not(bootstrap))]
#[doc(hidden)]
#[inline]
#[unstable(feature = "fmt_internals", reason = "internal to format_args!", issue = "none")]
#[rustc_const_unstable(feature = "const_fmt_arguments_new", issue = "none")]
pub const unsafe fn new_v1(
pieces: &'a [&'static str],
args: &'a [ArgumentV1<'a>],
) -> Arguments<'a> {
if pieces.len() < args.len() || pieces.len() > args.len() + 1 {
panic!("invalid args");
}
Arguments { pieces, fmt: None, args }
}
#[cfg(bootstrap)]
#[doc(hidden)]
#[inline]
#[unstable(feature = "fmt_internals", reason = "internal to format_args!", issue = "none")]
#[rustc_const_unstable(feature = "const_fmt_arguments_new", issue = "none")]
pub const fn new_v1(pieces: &'a [&'static str], args: &'a [ArgumentV1<'a>]) -> Arguments<'a> {
if pieces.len() < args.len() || pieces.len() > args.len() + 1 {
panic!("invalid args");
}
Arguments { pieces, fmt: None, args }
}

Expand All @@ -348,6 +365,19 @@ impl<'a> Arguments<'a> {
/// `CountIsParam` or `CountIsNextParam` has to point to an argument
/// created with `argumentusize`. However, failing to do so doesn't cause
/// unsafety, but will ignore invalid .
#[cfg(not(bootstrap))]
#[doc(hidden)]
#[inline]
#[unstable(feature = "fmt_internals", reason = "internal to format_args!", issue = "none")]
#[rustc_const_unstable(feature = "const_fmt_arguments_new", issue = "none")]
pub const unsafe fn new_v1_formatted(
pieces: &'a [&'static str],
args: &'a [ArgumentV1<'a>],
fmt: &'a [rt::v1::Argument],
) -> Arguments<'a> {
Arguments { pieces, fmt: Some(fmt), args }
}
#[cfg(bootstrap)]
#[doc(hidden)]
#[inline]
#[unstable(feature = "fmt_internals", reason = "internal to format_args!", issue = "none")]
Expand Down Expand Up @@ -1110,7 +1140,10 @@ pub fn write(output: &mut dyn Write, args: Arguments<'_>) -> Result {
match args.fmt {
None => {
// We can use default formatting parameters for all arguments.
for (arg, piece) in iter::zip(args.args, args.pieces) {
for (i, arg) in args.args.iter().enumerate() {
// SAFETY: args.args and args.pieces come from the same Arguments,
// which guarantees the indexes are always within bounds.
let piece = unsafe { args.pieces.get_unchecked(i) };
if !piece.is_empty() {
formatter.buf.write_str(*piece)?;
}
Expand All @@ -1121,7 +1154,10 @@ pub fn write(output: &mut dyn Write, args: Arguments<'_>) -> Result {
Some(fmt) => {
// Every spec has a corresponding argument that is preceded by
// a string piece.
for (arg, piece) in iter::zip(fmt, args.pieces) {
for (i, arg) in fmt.iter().enumerate() {
// SAFETY: fmt and args.pieces come from the same Arguments,
// which guarantees the indexes are always within bounds.
let piece = unsafe { args.pieces.get_unchecked(i) };
if !piece.is_empty() {
formatter.buf.write_str(*piece)?;
}
Expand Down
1 change: 1 addition & 0 deletions library/core/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@
//
// Language features:
#![feature(abi_unadjusted)]
#![feature(allow_internal_unsafe)]
#![feature(allow_internal_unstable)]
#![feature(asm)]
#![feature(associated_type_bounds)]
Expand Down
1 change: 1 addition & 0 deletions library/core/src/macros/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -828,6 +828,7 @@ pub(crate) mod builtin {
/// assert_eq!(s, format!("hello {}", "world"));
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
#[allow_internal_unsafe]
#[allow_internal_unstable(fmt_internals)]
#[rustc_builtin_macro]
#[macro_export]
Expand Down
10 changes: 9 additions & 1 deletion library/core/src/panicking.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,15 @@ pub fn panic(expr: &'static str) -> ! {
// truncation and padding (even though none is used here). Using
// Arguments::new_v1 may allow the compiler to omit Formatter::pad from the
// output binary, saving up to a few kilobytes.
panic_fmt(fmt::Arguments::new_v1(&[expr], &[]));
panic_fmt(
#[cfg(bootstrap)]
fmt::Arguments::new_v1(&[expr], &[]),
#[cfg(not(bootstrap))]
// SAFETY: Arguments::new_v1 is safe with exactly one str and zero args
unsafe {
fmt::Arguments::new_v1(&[expr], &[])
},
);
}

#[inline]
Expand Down
4 changes: 3 additions & 1 deletion src/test/debuginfo/rc_arc.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,5 +75,7 @@ fn main() {
let a1 = Arc::clone(&a);
let w2 = Arc::downgrade(&a);

print!(""); // #break
zzz(); // #break
}

fn zzz() { () }
10 changes: 6 additions & 4 deletions src/test/pretty/dollar-crate.pp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,11 @@

fn main() {
{
::std::io::_print(::core::fmt::Arguments::new_v1(&["rust\n"],
&match () {
() => [],
}));
::std::io::_print(match match () { () => [], } {
ref args => unsafe {
::core::fmt::Arguments::new_v1(&["rust\n"],
args)
}
});
};
}
56 changes: 33 additions & 23 deletions src/test/pretty/issue-4264.pp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,29 +32,39 @@
({
let res =
((::alloc::fmt::format as
for<'r> fn(Arguments<'r>) -> String {format})(((::core::fmt::Arguments::new_v1
as
fn(&[&'static str], &[ArgumentV1]) -> Arguments {Arguments::new_v1})((&([("test"
as
&str)]
as
[&str; 1])
as
&[&str; 1]),
(&(match (()
as
())
{
()
=>
([]
as
[ArgumentV1; 0]),
}
as
[ArgumentV1; 0])
as
&[ArgumentV1; 0]))
for<'r> fn(Arguments<'r>) -> String {format})((match (match (()
as
())
{
()
=>
([]
as
[ArgumentV1; 0]),
}
as
[ArgumentV1; 0])
{
ref args
=>
unsafe
{
((::core::fmt::Arguments::new_v1
as
unsafe fn(&[&'static str], &[ArgumentV1]) -> Arguments {Arguments::new_v1})((&([("test"
as
&str)]
as
[&str; 1])
as
&[&str; 1]),
(args
as
&[ArgumentV1; 0]))
as
Arguments)
}
}
as
Arguments))
as String);
Expand Down
6 changes: 3 additions & 3 deletions src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-84561.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,15 @@
24| 1| println!("{:?}", Foo(1));
25| 1|
26| 1| assert_ne!(Foo(0), Foo(5), "{}", if is_true { "true message" } else { "false message" });
^0 ^0 ^0
^0 ^0 ^0 ^0
27| 1| assert_ne!(
28| | Foo(0)
29| | ,
30| | Foo(5)
31| | ,
32| 0| "{}"
33| 0| ,
34| 0| if
33| | ,
34| | if
35| 0| is_true
36| | {
37| 0| "true message"
Expand Down
16 changes: 10 additions & 6 deletions src/test/ui/attributes/key-value-expansion.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,16 @@ LL | bug!();

error: unexpected token: `{
let res =
::alloc::fmt::format(::core::fmt::Arguments::new_v1(&[""],
&match (&"u8",) {
(arg0,) =>
[::core::fmt::ArgumentV1::new(arg0,
::core::fmt::Display::fmt)],
}));
::alloc::fmt::format(match match (&"u8",) {
(arg0,) =>
[::core::fmt::ArgumentV1::new(arg0,
::core::fmt::Display::fmt)],
} {
ref args => unsafe {
::core::fmt::Arguments::new_v1(&[""],
args)
}
});
res
}.as_str()`
--> $DIR/key-value-expansion.rs:48:23
Expand Down
8 changes: 7 additions & 1 deletion src/test/ui/unsafe/unsafe-around-compiler-generated-unsafe.mir.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,11 @@ note: the lint level is defined here
LL | #![deny(unused_unsafe)]
| ^^^^^^^^^^^^^

error: aborting due to previous error
error: unnecessary `unsafe` block
--> $DIR/unsafe-around-compiler-generated-unsafe.rs:13:5
|
LL | unsafe { println!("foo"); }
| ^^^^^^ unnecessary `unsafe` block

error: aborting due to 2 previous errors

3 changes: 3 additions & 0 deletions src/test/ui/unsafe/unsafe-around-compiler-generated-unsafe.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,7 @@ fn main() {
let _ = async {
unsafe { async {}.await; } //~ ERROR unnecessary `unsafe`
};

// `format_args!` expands with a compiler-generated unsafe block
unsafe { println!("foo"); } //~ ERROR unnecessary `unsafe`
}
8 changes: 7 additions & 1 deletion src/test/ui/unsafe/unsafe-around-compiler-generated-unsafe.thir.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,11 @@ note: the lint level is defined here
LL | #![deny(unused_unsafe)]
| ^^^^^^^^^^^^^

error: aborting due to previous error
error: unnecessary `unsafe` block
--> $DIR/unsafe-around-compiler-generated-unsafe.rs:13:5
|
LL | unsafe { println!("foo"); }
| ^^^^^^ unnecessary `unsafe` block

error: aborting due to 2 previous errors

Loading

0 comments on commit de42550

Please sign in to comment.