Make writeable_cmp_bytes a free function #5737

sffc · 2024-10-25T18:53:40Z

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

Manishearth

good call

robertbastian

I don't think this simplifies. There writeable argument is kind of fundamental so I think this should be a method.

sffc · 2024-10-25T20:56:46Z

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 · 2024-10-25T21:05:43Z

I agree on avoiding exposing functions like this for overriding

sffc · 2024-10-25T22:15:16Z

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 · 2024-10-26T20:04:19Z

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 · 2024-10-27T21:14:04Z

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.

robertbastian

Ok I buy the consistency argument. It would be nice if Rust had final trait methods for things like this (I think we discussed this at Rust conf at some point?).

robertbastian · 2024-10-28T12:10:33Z

utils/writeable/benches/writeable.rs

@@ -72,7 +72,7 @@ writeable::impl_display_with_writeable!(ComplexWriteable<'_>);

 const SHORT_STR: &str = "short";
 const MEDIUM_STR: &str = "this is a medium-length string";
-const LONG_STR: &str = "this string is very very very very very very very very very very very very very very very very very very very very very very very very long";
+const LONG_STR: &str = "this is a very very very very very very very very very very very very very very very very very very very very very very very very long string";


this change seems unnecessary and invalidates benchmark history.

I thought a longer overlap would make more meaningful benchmarks for cmp_bytes

Maybe we rename the benchmark, or have LONG_STR_DIVERGE_EARLY and LONG_STR_DIVERGE_LATE

I thought a longer overlap would make more meaningful benchmarks for cmp_bytes

And it totally is. Just use a new const for it so all the preexisting benchmarks are kept stable.

I don't really see the problem to be solved (the new string is only 2 bytes longer than the old one, and people who track the benchmarks over time can see this commit as the culprit) and prefer merging this as-is but I can make @Manishearth's suggested change if necessary.

people who track the benchmarks over time can see this commit as the culprit

If you're looking at the graph you don't know whether it's a benchmark setup change, or a behaviour change. All you'll see is "Make writeable_cmp_bytes a free function", which shouldn't change any benchmarks.

I made the string the same length as before. (The bigger change requires a setup with a code editor which I don't have at the moment but will in about an hour.) I still prefer landing this to avoid the context switch later.

ok I didn't hear back about whether my compromise was sufficient so I went ahead and implemented the solution that I believe addresses @robertbastian's blocking concern.

robertbastian · 2024-10-28T12:13:27Z

utils/writeable/src/cmp.rs

+///
+/// This returns a lexicographical comparison, the same as if the Writeable
+/// were first converted to a String and then compared with `Ord`. For a
+/// locale-sensitive string ordering, use an ICU4X Collator.


writeable should be independent of ICU4X, not sure we should recommend this here.

This is pre-existing text.

I changed the text slightly. I recommend to icu_collator as an external crate that does localized string sorting in the same vein as Jiff might recommend icu_calendar as an external crate that does non-Gregorian calendars.

Yeah, that works.

Not super happy with this, but won't block the PR as this is preexisting. impl Ord for str, and str I guess don't go into locale-aware collation either, and I consider this crate an alternative to those core types.

robertbastian

.

sffc added 5 commits October 25, 2024 11:11

change string to have a longer prefix overlap

7291906

Add writeable_cmp_bytes benches

53dd193

Remove specialized writeable_cmp_bytes impls

4294217

Change writeable_cmp_bytes to a free function

5d1f4f3

Replace call sites across icu4x

9832f3f

sffc requested review from robertbastian, Manishearth, zbraniecki and nciric as code owners October 25, 2024 18:53

Make the helper type more private

e0687aa

Manishearth previously approved these changes Oct 25, 2024

View reviewed changes

fix docs test

0683114

sffc dismissed Manishearth’s stale review via 0683114 October 25, 2024 19:45

sffc mentioned this pull request Oct 25, 2024

Add writeable::write_or_ref and use it in icu_locale_core #5738

Merged

robertbastian requested changes Oct 25, 2024

View reviewed changes

sffc added 2 commits October 25, 2024 13:51

Changelog

be4dbaa

Add link to PR

c82932d

sffc requested a review from a team as a code owner October 25, 2024 20:54

sffc requested a review from robertbastian October 25, 2024 20:56

Manishearth previously approved these changes Oct 26, 2024

View reviewed changes

robertbastian reviewed Oct 28, 2024

View reviewed changes

Language around ICU4X

4aec7dd

sffc dismissed Manishearth’s stale review via 4aec7dd October 28, 2024 17:02

sffc requested a review from robertbastian October 28, 2024 17:03

Manishearth previously approved these changes Oct 28, 2024

View reviewed changes

robertbastian reviewed Oct 28, 2024

View reviewed changes

sffc requested a review from robertbastian October 28, 2024 17:14

Make the string the same length as before

278d042

sffc dismissed Manishearth’s stale review via 278d042 October 28, 2024 17:15

sffc added 2 commits October 28, 2024 14:02

Revert change to LONG_STR

04eb828

Add LONG_OVERLAP_STR

fdbbc16

robertbastian approved these changes Oct 28, 2024

View reviewed changes

sffc merged commit 1159228 into unicode-org:main Oct 28, 2024
28 checks passed

sffc deleted the writeable-util branch October 28, 2024 21:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make writeable_cmp_bytes a free function #5737

Make writeable_cmp_bytes a free function #5737

sffc commented Oct 25, 2024

Manishearth left a comment

robertbastian left a comment

sffc commented Oct 25, 2024

Manishearth commented Oct 25, 2024

sffc commented Oct 25, 2024

sffc commented Oct 26, 2024

sffc commented Oct 27, 2024

robertbastian left a comment

robertbastian Oct 28, 2024

sffc Oct 28, 2024

Manishearth Oct 28, 2024

robertbastian Oct 28, 2024

sffc Oct 28, 2024

robertbastian Oct 28, 2024

sffc Oct 28, 2024

sffc Oct 28, 2024

robertbastian Oct 28, 2024

Manishearth Oct 28, 2024

sffc Oct 28, 2024

sffc Oct 28, 2024

Manishearth Oct 28, 2024

robertbastian Oct 28, 2024

robertbastian left a comment

Make writeable_cmp_bytes a free function #5737

Make writeable_cmp_bytes a free function #5737

Conversation

sffc commented Oct 25, 2024

Manishearth left a comment

Choose a reason for hiding this comment

robertbastian left a comment

Choose a reason for hiding this comment

sffc commented Oct 25, 2024

Manishearth commented Oct 25, 2024

sffc commented Oct 25, 2024

sffc commented Oct 26, 2024

sffc commented Oct 27, 2024

robertbastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertbastian left a comment

Choose a reason for hiding this comment