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

sffc · 2024-10-25T20:23:53Z

Follow-up to #5727
Depends on #5737
Replaces #5733
See #2748

I am adding an additional utility function to the writeable crate with a similar architecture to writeable::cmp_bytes. This one, writeable::write_or_ref, makes certain normalization operations more efficient.

sffc · 2024-10-25T21:54:08Z

Please note, I wrote unsafe code in 68c2b97 and then re-wrote it in d6485c8.

This PR is small enough that reviewing it at once is probably more efficient.

sffc · 2024-10-25T22:17:39Z

Our utility functions are nicely organized in the docs:

https://icu4x-pr-artifacts.storage.googleapis.com/gha/4d63b3a3d37f9f564be58be2e3059744f7d7b514/rustdoc/writeable/index.html#functions

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

utils/writeable/src/write_or_ref.rs

+///     Cow::Borrowed("Hello, Alice!")
+/// ));
+///
+/// // Junk at the end is ignored:


Suggested change

/// // Junk at the end is ignored:

/// // Borrowing can use a prefix:

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

utils/writeable/src/write_or_ref.rs

+///     Cow::Owned(s) if s == "this is uppercase"
+/// ));
+/// ```
+pub fn write_or_ref<'a>(writeable: &impl Writeable, reference_bytes: &'a [u8]) -> Cow<'a, str> {


I'm not happy with this name. Usually methods called write write to one of the arguments, and don't do any allocation themselves, like this does. I think write_to_string_or_borrow would be a more accurate name, and would align more with write_to_string, which is meant to use it.

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

utils/writeable/src/write_or_ref.rs

+        let owned: &mut String = match self {
+            SliceOrString::Slice(slice) => {
+                if slice.try_push(other) {
+                    return Ok(());
+                }
+                // We failed to match. Convert to owned, put it in the field,
+                // and get it out again.
+                let valid_str = slice.validated_as_str();
+                let owned = String::from(valid_str);
+                *self = SliceOrString::String(owned);
+                let SliceOrString::String(owned) = self else {
+                    unreachable!()
+                };
+                owned
+            }
+            SliceOrString::String(owned) => owned,
+        };
+        owned.write_str(other)


this is jumping through a lot of hoops to not duplicate line 75. I think the code would be a lot easier if you did that:

Suggested change

let owned: &mut String = match self {

SliceOrString::Slice(slice) => {

if slice.try_push(other) {

return Ok(());

}

// We failed to match. Convert to owned, put it in the field,

// and get it out again.

let valid_str = slice.validated_as_str();

let owned = String::from(valid_str);

*self = SliceOrString::String(owned);

let SliceOrString::String(owned) = self else {

unreachable!()

};

owned

}

SliceOrString::String(owned) => owned,

};

owned.write_str(other)

match self {

SliceOrString::Slice(slice) => {

if !slice.try_push(other) {

// We failed to match. Convert to owned.

let valid_str = slice.validated_as_str();

let owned = format!("{valid_str}{other});

*self = SliceOrString::String(owned);

Ok(())

}

SliceOrString::String(owned) => owned.write_str(other),

}

I don't think it's a lot of hoops: it is a standard "base case" coding style. That said, I think your code is more readable, except I don't like using format!. I'll push up something similar.

For this Rust program:

#[inline(never)] fn string_from_pieces_format(a: &str, b: &str) -> String { std::format!("{a}{b}") } #[inline(never)] fn string_from_pieces_manual(a: &str, b: &str) -> String { let mut s = String::with_capacity(a.len() + b.len()); s.push_str(a); s.push_str(b); s } fn main() { let s1 = string_from_pieces_format("hello", "world"); let s2 = string_from_pieces_manual("hello", "world"); println!("{s1} {s2}"); }

Here is the ASM (https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=dff4d0ae823abc03d88fad4b84b56a7d):

std::rt::lang_start: pushq %rax movl %ecx, %r8d movq %rdx, %rcx movq %rsi, %rdx movq %rdi, (%rsp) leaq .L__unnamed_1(%rip), %rsi movq %rsp, %rdi callq *std::rt::lang_start_internal@GOTPCREL(%rip) popq %rcx retq std::rt::lang_start::{{closure}}: pushq %rax movq (%rdi), %rdi callq std::sys::backtrace::__rust_begin_short_backtrace xorl %eax, %eax popq %rcx retq std::sys::backtrace::__rust_begin_short_backtrace: pushq %rax callq *%rdi #APP #NO_APP popq %rax retq <&T as core::fmt::Display>::fmt: movq %rsi, %rdx movq (%rdi), %rax movq 8(%rdi), %rsi movq %rax, %rdi jmpq *<str as core::fmt::Display>::fmt@GOTPCREL(%rip) core::ops::function::FnOnce::call_once{{vtable.shim}}: pushq %rax movq (%rdi), %rdi callq std::sys::backtrace::__rust_begin_short_backtrace xorl %eax, %eax popq %rcx retq <alloc::string::String as core::fmt::Display>::fmt: movq %rsi, %rdx movq 8(%rdi), %rax movq 16(%rdi), %rsi movq %rax, %rdi jmpq *<str as core::fmt::Display>::fmt@GOTPCREL(%rip) playground::string_from_pieces_format: subq $120, %rsp leaq .L__unnamed_2(%rip), %rax movq %rax, 8(%rsp) movq $5, 16(%rsp) leaq .L__unnamed_3(%rip), %rax movq %rax, 24(%rsp) movq $5, 32(%rsp) leaq 8(%rsp), %rax movq %rax, 40(%rsp) leaq <&T as core::fmt::Display>::fmt(%rip), %rax movq %rax, 48(%rsp) leaq 24(%rsp), %rcx movq %rcx, 56(%rsp) movq %rax, 64(%rsp) leaq .L__unnamed_4(%rip), %rax movq %rax, 72(%rsp) movq $2, 80(%rsp) movq $0, 104(%rsp) leaq 40(%rsp), %rax movq %rax, 88(%rsp) movq $2, 96(%rsp) leaq 72(%rsp), %rsi callq *alloc::fmt::format::format_inner@GOTPCREL(%rip) addq $120, %rsp retq playground::string_from_pieces_manual: pushq %rbx movq %rdi, %rbx movq __rust_no_alloc_shim_is_unstable@GOTPCREL(%rip), %rax movzbl (%rax), %eax movl $10, %edi movl $1, %esi callq *__rust_alloc@GOTPCREL(%rip) testq %rax, %rax je .LBB7_2 movb $111, 4(%rax) movl $1819043176, (%rax) movl $1819438967, 5(%rax) movb $100, 9(%rax) movq $10, (%rbx) movq %rax, 8(%rbx) movq $10, 16(%rbx) popq %rbx retq .LBB7_2: movl $1, %edi movl $10, %esi callq *alloc::raw_vec::handle_error@GOTPCREL(%rip) playground::main: pushq %r14 pushq %rbx subq $136, %rsp leaq 8(%rsp), %rbx movq %rbx, %rdi callq playground::string_from_pieces_format leaq 32(%rsp), %r14 movq %r14, %rdi callq playground::string_from_pieces_manual movq %rbx, 56(%rsp) leaq <alloc::string::String as core::fmt::Display>::fmt(%rip), %rax movq %rax, 64(%rsp) movq %r14, 72(%rsp) movq %rax, 80(%rsp) leaq .L__unnamed_5(%rip), %rax movq %rax, 88(%rsp) movq $3, 96(%rsp) movq $0, 120(%rsp) leaq 56(%rsp), %rax movq %rax, 104(%rsp) movq $2, 112(%rsp) leaq 88(%rsp), %rdi callq *std::io::stdio::_print@GOTPCREL(%rip) movq 32(%rsp), %rsi testq %rsi, %rsi je .LBB8_4 movq 40(%rsp), %rdi movl $1, %edx callq *__rust_dealloc@GOTPCREL(%rip) .LBB8_4: movq 8(%rsp), %rsi testq %rsi, %rsi je .LBB8_6 movq 16(%rsp), %rdi movl $1, %edx callq *__rust_dealloc@GOTPCREL(%rip) .LBB8_6: addq $136, %rsp popq %rbx popq %r14 retq movq %rax, %rbx movq 32(%rsp), %rsi testq %rsi, %rsi je .LBB8_8 movq 40(%rsp), %rdi movl $1, %edx callq *__rust_dealloc@GOTPCREL(%rip) jmp .LBB8_8 movq %rax, %rbx .LBB8_8: movq 8(%rsp), %rsi testq %rsi, %rsi je .LBB8_10 movq 16(%rsp), %rdi movl $1, %edx callq *__rust_dealloc@GOTPCREL(%rip) .LBB8_10: movq %rbx, %rdi callq _Unwind_Resume@PLT main: pushq %rax movq %rsi, %rcx movslq %edi, %rdx leaq playground::main(%rip), %rax movq %rax, (%rsp) leaq .L__unnamed_1(%rip), %rsi movq %rsp, %rdi xorl %r8d, %r8d callq *std::rt::lang_start_internal@GOTPCREL(%rip) popq %rcx retq .L__unnamed_1: .asciz "\000\000\000\000\000\000\000\000\b\000\000\000\000\000\000\000\b\000\000\000\000\000\000" .quad core::ops::function::FnOnce::call_once{{vtable.shim}} .quad std::rt::lang_start::{{closure}} .quad std::rt::lang_start::{{closure}} .L__unnamed_4: .quad 1 .zero 8 .quad 1 .zero 8 .L__unnamed_2: .ascii "hello" .L__unnamed_3: .ascii "world" .L__unnamed_6: .byte 32 .L__unnamed_7: .byte 10 .L__unnamed_5: .quad 1 .zero 8 .quad .L__unnamed_6 .asciz "\001\000\000\000\000\000\000" .quad .L__unnamed_7 .asciz "\001\000\000\000\000\000\000"

So it appears avoiding format! is worthwhile.

Cannot parse that atm. Just make sure to reserve capacity once.

Manishearth

Looks good overall.

No strong opinion on naming, weakly agree that write_or_ref isn't ideal, but don't have antyhing better.

Needs more structured safety comments but I tried to help with that.

It's unfortunate this needs unsafe at all, the str version of this would be purely safe. But we do want Potentially versions.

Manishearth · 2024-10-28T16:16:33Z

utils/writeable/src/write_or_ref.rs

+
+impl<'a> PartiallyValidatedUtf8<'a> {
+    fn new(slice: &'a [u8]) -> Self {
+        Self { slice, offset: 0 }


nit: safety comment:

/// Safety: Field invariants maintained here trivially: /// 1. The offset 0 is ≤ all possible lengths of slice /// 2. The slice contains nothing up to the offset zero

We typically want similar safety comments for whenever one of the fields is mutated.

Manishearth · 2024-10-28T16:17:16Z

utils/writeable/src/write_or_ref.rs

+            // Safety Invariant: `valid_str` is valid UTF-8, and we are
+            // incrementing the offset to cover bytes equal to `valid_str`
+            self.offset += valid_str.len();
+            true


I'd say "Safety: Field invariants maintained here by..." and use a list like above

Manishearth · 2024-10-28T16:17:38Z

utils/writeable/src/write_or_ref.rs

+    /// Return the validated portion as `&str`.
+    fn validated_as_str(&self) -> &'a str {
+        debug_assert!(self.offset <= self.slice.len());
+        // Safety: self.offset is a valid end index in a range


nit: I'd suffix both of these comments with (from field invariant), or somethign similar, basically indicating where the invariant comes from

Manishearth · 2024-10-28T16:19:37Z

utils/writeable/src/write_or_ref.rs

+}
+
+/// Writes the contents of a `Writeable` to a string, returning a reference
+/// to a slice if it matches, and allocating a string otherwise.


Suggested change

/// to a slice if it matches, and allocating a string otherwise.

/// to a slice if it matches the provided reference bytes, and allocating a string otherwise.

sffc · 2024-10-28T21:39:14Z

I think I addressed all comments other than the function name. Happy to entertain suggestions.

Bikeshed:

writeable::write_or_ref (current)
writeable::write_to_string_or_borrow (@robertbastian suggestion)
writeable::to_string_or_borrow
writeable::to_string_or_ref
writeable::write_or_borrow
writeable::or_borrow
writeable::or_ref

A problem with write_to_string_or_borrow is that write_to_string is already defined to borrow when able. I've never been super happy with the name of that trait function because of this.

sffc · 2024-10-28T21:41:43Z

It's unfortunate this needs unsafe at all, the str version of this would be purely safe. But we do want Potentially versions.

I would argue that the &[u8] version is superior to an all-&str version not only because of PotentialUtf8 but also because (1) it could be used when you have some other blob data you can reference, like postcard or something, and (2) the &str functions are a little annoying because they all return Options due to code point boundaries, so I think we'd need to have an unwrap or get_unchecked anyway.

robertbastian · 2024-10-28T21:43:29Z

A problem with write_to_string_or_borrow is that write_to_string is already defined to borrow when able. I've never been super happy with the name of that trait function because of this.

Yeah, write_to_string is defined to borrow when able, and writeable::write_to_string_or_borrow is how that can be implemented.

dpulls · 2024-10-28T21:45:29Z

🎉 All dependencies have been resolved !

sffc · 2024-10-29T01:15:52Z

I'll wait to merge this until we discuss the name.

sffc · 2024-10-29T15:38:42Z

Naming choice: writeable::to_string_or_borrow

LGTM: @Manishearth @robertbastian (@sffc)

sffc added 7 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

Make the helper type more private

e0687aa

fix docs test

0683114

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

sffc added 8 commits October 25, 2024 13:51

Changelog

be4dbaa

Add link to PR

c82932d

Initial impl of write_or_ref

68c2b97

Fix and add tests

6b4da0e

Use the new fn in icu_locale_core

d883aaf

Refactor

d6485c8

get_unchecked

8e5e907

Changelog

4d63b3a

sffc force-pushed the write-or-ref branch from 5e0587f to 4d63b3a Compare October 25, 2024 20:54

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

This was referenced Oct 25, 2024

Make writeable_cmp_bytes a free function #5737

Merged

Don't allocate to check canonicalization result #5733

Closed

robertbastian reviewed Oct 28, 2024

View reviewed changes

Manishearth reviewed Oct 28, 2024

View reviewed changes

sffc added 3 commits October 28, 2024 14:10

Apply suggestion in SliceOrString::write_str

c027ca7

Use String instead of fmt::Formatter

946cacf

Improved safety comments

db43027

Apply documentation suggestions

508cedc

Merge branch 'main' into write-or-ref

e896637

sffc requested a review from Manishearth October 28, 2024 21:47

Manishearth previously approved these changes Oct 29, 2024

View reviewed changes

sffc added the discuss-priority Discuss at the next ICU4X meeting label Oct 29, 2024

to_string_or_borrow

e9e08a5

sffc dismissed Manishearth’s stale review via e9e08a5 October 29, 2024 20:47

robertbastian approved these changes Oct 29, 2024

View reviewed changes

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

sffc deleted the write-or-ref branch October 29, 2024 22:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sffc commented Oct 25, 2024 •

edited

Loading

sffc commented Oct 25, 2024

sffc commented Oct 25, 2024 •

edited

Loading

robertbastian Oct 28, 2024

robertbastian Oct 28, 2024

robertbastian Oct 28, 2024

sffc Oct 28, 2024

sffc Oct 28, 2024

robertbastian Oct 28, 2024

Manishearth left a comment •

edited

Loading

Manishearth Oct 28, 2024

Manishearth Oct 28, 2024

Manishearth Oct 28, 2024

Manishearth Oct 28, 2024

sffc commented Oct 28, 2024

sffc commented Oct 28, 2024

robertbastian commented Oct 28, 2024

dpulls bot commented Oct 28, 2024

sffc commented Oct 29, 2024

sffc commented Oct 29, 2024

	/// // Junk at the end is ignored:
	/// // Borrowing can use a prefix:

	/// to a slice if it matches, and allocating a string otherwise.
	/// to a slice if it matches the provided reference bytes, and allocating a string otherwise.

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

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

Conversation

sffc commented Oct 25, 2024 • edited Loading

sffc commented Oct 25, 2024

sffc commented Oct 25, 2024 • edited Loading

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

Manishearth left a comment • edited Loading

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

sffc commented Oct 28, 2024

sffc commented Oct 28, 2024

robertbastian commented Oct 28, 2024

dpulls bot commented Oct 28, 2024

sffc commented Oct 29, 2024

sffc commented Oct 29, 2024

sffc commented Oct 25, 2024 •

edited

Loading

sffc commented Oct 25, 2024 •

edited

Loading

Manishearth left a comment •

edited

Loading