Add tinystr-neo to experimental/ #1508

Manishearth · 2022-01-14T21:42:12Z

This is a clean attempt at tinystr, using const fn and fixed size arrays.

This should address zbraniecki/tinystr#45 / #881

The general idea is, instead of encoding strings as digits, we encode them as byte arrays. This has many advantages:

No mucking about with endianness
We can support things like TinyAsciiStr<5> or TinyAsciiStr<100>, no restrictions on N
ULE gets cleaner

A potential downside is that these are less aligned (we can write TinyStr4/etc wrappers that are better aligned), and .deref()/deserialize() may not be as efficient (we might need to benchmark this).

This is still kinda WIP, but I'm not sure how much time I'll have to work on this so I'm landing what I have so far so that others can help out. In particular I'd love help with tests, benches, and docs.

Manishearth · 2022-01-14T21:43:11Z

experimental/tinystr_neo/src/ascii.rs

+
+        let mut out = [0; N];
+        let mut i = 0;
+        while i < bytes.len() {


note: can't be a for loop in const context

Manishearth · 2022-01-14T21:44:07Z

experimental/tinystr_neo/src/ascii.rs

+    }
+
+    pub fn len(&self) -> usize {
+        self.bytes.iter().position(|x| *x == 0).unwrap_or(N)


note: previously we were able to use .trailing_zeroes()/leading_zeroes() which are a single instruction. If rustc unrolls this loop for small N we should also be fine here, but I'm not sure if that's happening, we should bench this.

Yeah; this is one of the things I was wondering how we would implement efficiently. We should definitely test. I think this would be a good thing for IAI as opposed to Clippy.

Manishearth · 2022-01-14T21:44:43Z

experimental/tinystr_neo/src/serde.rs

+    }
+
+    #[inline]
+    fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error>


note: this is likely not as efficient as the deserialization from the tinystr crate.

We could add TinyStr4/etc as aligned wrappers that serialize to little-endian numbers.

I'm not too worried about impl Deserialize on TinyAsciiStr itself, because that one is only used when TinyAsciiStr is at the top level of a data struct. I anticipate the much more common case to be when it is in a ZeroVec, where the ULE impl will be used instead of the Deserialize impl.

sffc · 2022-01-15T05:43:58Z

experimental/tinystr_neo/src/macros.rs

+// (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ).
+
+#[macro_export]
+macro_rules! tinystr {


Thought: We maybe don't need the macro any more. People can just use the const function constructors directly. Fewer imports and friendlier to the compiler.

So as mentioned in #1511 (comment) , I think it's important to give users a convenient way to call this in a const context, given that it's rare and unusual for Rust programmers to const locals wherever possible. Unlike that PR, there will be no exported consts for people to use this, so I'd prefer to keep the macro around.

MBEs (non proc macros, like this one) are pretty friendly to the compiler; and this is going to be even smaller once I rebase over the rust update. I don't find that argument that compelling.

Yeah, macro_rules is definitely an improvement over a proc macro. Okay, I'm convinced.

experimental/tinystr_neo/tests/serde.rs

sffc

LGTM and please update the PR to Rust 1.58 so we can get rid of the runtime unwrap

zbraniecki · 2022-01-16T20:37:52Z

Manish - do you plan to measure perf before merging o before switching callsites to it?

Manishearth · 2022-01-16T21:24:13Z

@zbraniecki before switching callsites. The crate is WIP (and experimental/), I just want to land what I have so far so that others (you?? 🥺) can help with docs, benches, API surface, etc.

jira-pull-request-webhook · 2022-01-17T15:48:15Z

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

sffc · 2022-01-17T19:59:35Z

experimental/tinystr_neo/src/macros.rs

+        const TINYSTR_MACRO_CONST: $crate::TinyAsciiStr<$n> =
+            $crate::TinyAsciiStr::from_str_panicky($s);


Suggestion: does it work if you do this?

Suggested change

const TINYSTR_MACRO_CONST: $crate::TinyAsciiStr<$n> =

$crate::TinyAsciiStr::from_str_panicky($s);

const TINYSTR_MACRO_CONST: $crate::TinyAsciiStr<$n> =

$crate::TinyAsciiStr::from_str($s).unwrap();

or perhaps

Suggested change

const TINYSTR_MACRO_CONST: $crate::TinyAsciiStr<$n> =

$crate::TinyAsciiStr::from_str_panicky($s);

const TINYSTR_MACRO_CONST: $crate::TinyAsciiStr<$n> =

$crate::TinyAsciiStr::from_str($s).ok().expect("Invalid tinystr");

(with the unwrap inside the const context)

No, unwrap isn't stably const

Not even Option::unwrap? oh well... how about

const TINYSTR_MACRO_CONST: $crate::TinyAsciiStr<$n> = match $crate::TinyAsciiStr::from_str($s) { Ok(s) => s, Err(_) => panic!("{}", $s) // Invalid tinystr }

Or, keep it the way it is. It's just that it might be nice to not have a panicky library function.

Yeah I didn't like that function either, moved

sffc · 2022-01-18T02:40:15Z

experimental/tinystr_neo/src/macros.rs

+            match $crate::TinyAsciiStr::from_bytes($s.as_bytes()) {
+                Ok(s) => s,
+                // Cannot format the error since formatting isn't const yet
+                Err(_) => panic!(concat!("Failed to construct tinystr from ", $s)),


Praise: Good idea to use concat! to format a nicer message

Manishearth requested a review from a team as a code owner January 14, 2022 21:42

Manishearth requested review from sffc and zbraniecki January 14, 2022 21:42

Manishearth commented Jan 14, 2022

View reviewed changes

sffc reviewed Jan 15, 2022

View reviewed changes

sffc requested changes Jan 15, 2022

View reviewed changes

experimental/tinystr_neo/tests/serde.rs Show resolved Hide resolved

experimental/tinystr_neo/tests/serde.rs Outdated Show resolved Hide resolved

sffc previously approved these changes Jan 16, 2022

View reviewed changes

Manishearth added 8 commits January 17, 2022 07:48

Add tinystr_neo experimental crate

8c13b98

Basic TinyAsciiStr type

ad88484

Add serde impls

3044449

Add tests for serde

1361678

fix var name

510c57a

rm bytes

73a799d

fmt

6bfbfc3

tidy

2cdeab3

Manishearth dismissed sffc’s stale review via 2cdeab3 January 17, 2022 15:48

Manishearth force-pushed the tinystr-neo branch from fbeea77 to 2cdeab3 Compare January 17, 2022 15:48

Make macro panic at compile time

347c955

sffc previously approved these changes Jan 17, 2022

View reviewed changes

clippy

601787d

Manishearth dismissed sffc’s stale review via 601787d January 17, 2022 20:29

Manishearth requested a review from sffc January 17, 2022 20:29

move panic

d009c19

sffc previously approved these changes Jan 18, 2022

View reviewed changes

clip

2d9e801

Manishearth dismissed sffc’s stale review via 2d9e801 January 18, 2022 07:59

sffc approved these changes Jan 18, 2022

View reviewed changes

Manishearth merged commit 30a3f07 into unicode-org:main Jan 18, 2022

Manishearth deleted the tinystr-neo branch January 18, 2022 18:06

Manishearth restored the tinystr-neo branch January 19, 2022 21:20

Manishearth mentioned this pull request Jan 20, 2022

Implement and switch to tinystr_neo #1527

Closed

Manishearth deleted the tinystr-neo branch February 10, 2022 17:41

Manishearth mentioned this pull request Feb 18, 2022

Figure out endianness story zbraniecki/tinystr#45

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tinystr-neo to experimental/ #1508

Add tinystr-neo to experimental/ #1508

Manishearth commented Jan 14, 2022

Manishearth Jan 14, 2022

Manishearth Jan 14, 2022

sffc Jan 15, 2022

Manishearth Jan 14, 2022

Manishearth Jan 14, 2022

sffc Jan 15, 2022

sffc Jan 15, 2022

Manishearth Jan 16, 2022

sffc Jan 16, 2022

sffc left a comment

zbraniecki commented Jan 16, 2022

Manishearth commented Jan 16, 2022 •

edited

Loading

jira-pull-request-webhook bot commented Jan 17, 2022

sffc Jan 17, 2022

sffc Jan 17, 2022

Manishearth Jan 17, 2022 •

edited

Loading

sffc Jan 17, 2022

Manishearth Jan 17, 2022

sffc Jan 18, 2022

		const TINYSTR_MACRO_CONST: $crate::TinyAsciiStr<$n> =
		$crate::TinyAsciiStr::from_str_panicky($s);

Add tinystr-neo to experimental/ #1508

Add tinystr-neo to experimental/ #1508

Conversation

Manishearth commented Jan 14, 2022

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

sffc left a comment

Choose a reason for hiding this comment

zbraniecki commented Jan 16, 2022

Manishearth commented Jan 16, 2022 • edited Loading

jira-pull-request-webhook bot commented Jan 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth Jan 17, 2022 • 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

Manishearth commented Jan 16, 2022 •

edited

Loading

Manishearth Jan 17, 2022 •

edited

Loading