unicode-org / icu4x

Solving i18n for client-side and resource-constrained environments.
https://icu4x.unicode.org
Other
1.38k stars 178 forks source link

Make writeable_cmp_bytes a free function #5737

Closed sffc closed 3 weeks ago

sffc commented 3 weeks ago

It simplifies writeable impls. There is no impact on benchmarks, either the new microbenchmark I added or the higher-level strict_cmp benchmark in the icu_locale_core crate:

     Running benches/langid.rs
langid/compare/strict_cmp/langid
                        time:   [271.42 ns 272.76 ns 274.20 ns]
                        change: [-0.2857% +0.0019% +0.3042%] (p = 0.99 > 0.05)
                        No change in performance detected.
Found 12 outliers among 100 measurements (12.00%)
  4 (4.00%) high mild
  8 (8.00%) high severe

     Running benches/locale.rs
locale/compare/strict_cmp/locale
                        time:   [370.74 ns 371.09 ns 371.44 ns]
                        change: [-0.4829% -0.1979% +0.0605%] (p = 0.16 > 0.05)
                        No change in performance detected.
Found 8 outliers among 100 measurements (8.00%)
  2 (2.00%) high mild
  6 (6.00%) high severe
sffc commented 3 weeks ago

The trait should only have things that can actually be specialized. Defaulted trait functions create maintenance burden and code bloat. I want to move more toward free utility functions (#5738).

Manishearth commented 3 weeks ago

I agree on avoiding exposing functions like this for overriding

sffc commented 3 weeks ago

Note that the assert_writeable_eq impl gets a fair bit shorter with this change, because we reduced the surface of trait impl that we need to test.

sffc commented 3 weeks ago

I think a free function is good, but if you don't like the ergonomics, it could be added by trait injection

pub trait WriteableCmpBytes {
    pub fn writeable_cmp_bytes(...)
}
impl<T: Writeable + ?Sized> WriteableCmpBytes for T { ... }

But I prefer free functions.

sffc commented 3 weeks ago

Additional context: I had originally made this a function on the trait because I had thought that it could be specialized for certain types. However, a year in, we haven't found cases where a concrete type can offer a more efficient implementation of this function than the default, according to the benchmarks in this PR. So, there is no longer a technical reason why it needs to be a trait function.