Skip to content

Commit

Permalink
ruff_formatter: remove the mono-morphed code
Browse files Browse the repository at this point in the history
This replaces the raw pointer type erasure with safe dynamic dispatch.
I'm a little unsure about this since I don't quite understand why the
code was written the way it was. In any case, I did a before-and-after
benchmark comparison:

    $ critcmp main test
    group                            main                                   test
    -----                            ----                                   ----
    formatter/large/dataset.py       1.00   1306.0±9.42µs    31.2 MB/sec    1.02   1328.5±6.89µs    30.6 MB/sec
    formatter/numpy/ctypeslib.py     1.00    271.3±2.78µs    61.4 MB/sec    1.02    276.3±1.88µs    60.3 MB/sec
    formatter/numpy/globals.py       1.00     21.8±0.18µs   135.1 MB/sec    1.04     22.7±0.18µs   129.9 MB/sec
    formatter/pydantic/types.py      1.00    522.1±4.08µs    48.8 MB/sec    1.01    529.7±3.90µs    48.1 MB/sec
    formatter/unicode/pypinyin.py    1.00     80.7±0.30µs    52.0 MB/sec    1.02     82.2±0.28µs    51.1 MB/sec

I ran the benchmarks a few times, and the same result persisted. So it
does seem like there is a very tiny regression here. Even so, I think
it's worth it if we can remove the use of `unsafe` (and assuming there
aren't any bigger regressions).
  • Loading branch information
BurntSushi committed Nov 28, 2023
1 parent 5135cd1 commit 8900f7b
Showing 1 changed file with 7 additions and 34 deletions.
41 changes: 7 additions & 34 deletions crates/ruff_formatter/src/arguments.rs
Original file line number Diff line number Diff line change
@@ -1,23 +1,9 @@
use super::{Buffer, Format, Formatter};
use crate::FormatResult;
use std::ffi::c_void;
use std::marker::PhantomData;

/// Mono-morphed type to format an object. Used by the [`crate::format`!], [`crate::format_args`!], and
/// [`crate::write`!] macros.
///
/// This struct is similar to a dynamic dispatch (using `dyn Format`) because it stores a pointer to the value.
/// However, it doesn't store the pointer to `dyn Format`'s vtable, instead it statically resolves the function
/// pointer of `Format::format` and stores it in `formatter`.
/// A convenience wrapper for representing a formattable argument.
pub struct Argument<'fmt, Context> {
/// The value to format stored as a raw pointer where `lifetime` stores the value's lifetime.
value: *const c_void,

/// Stores the lifetime of the value. To get the most out of our dear borrow checker.
lifetime: PhantomData<&'fmt ()>,

/// The function pointer to `value`'s `Format::format` method
formatter: fn(*const c_void, &mut Formatter<'_, Context>) -> FormatResult<()>,
value: &'fmt dyn Format<Context>,
}

impl<Context> Clone for Argument<'_, Context> {
Expand All @@ -28,32 +14,19 @@ impl<Context> Clone for Argument<'_, Context> {
impl<Context> Copy for Argument<'_, Context> {}

impl<'fmt, Context> Argument<'fmt, Context> {
/// Called by the [ruff_formatter::format_args] macro. Creates a mono-morphed value for formatting
/// an object.
/// Called by the [ruff_formatter::format_args] macro.
#[doc(hidden)]
#[inline]
pub fn new<F: Format<Context>>(value: &'fmt F) -> Self {
#[inline]
fn formatter<F: Format<Context>, Context>(
ptr: *const c_void,
fmt: &mut Formatter<Context>,
) -> FormatResult<()> {
// SAFETY: Safe because the 'fmt lifetime is captured by the 'lifetime' field.
#[allow(unsafe_code)]
F::fmt(unsafe { &*ptr.cast::<F>() }, fmt)
}

Self {
value: (value as *const F).cast::<std::ffi::c_void>(),
lifetime: PhantomData,
formatter: formatter::<F, Context>,
}
Self { value }
}

/// Formats the value stored by this argument using the given formatter.
#[inline]
// Seems to only be triggered on wasm32 and looks like a false positive?
#[allow(clippy::trivially_copy_pass_by_ref)]
pub(super) fn format(&self, f: &mut Formatter<Context>) -> FormatResult<()> {
(self.formatter)(self.value, f)
self.value.fmt(f)
}
}

Expand Down

0 comments on commit 8900f7b

Please sign in to comment.