Switch to TinyStr #7

zbraniecki · 2019-08-07T18:22:32Z

This is a port of https://github.com/projectfluent/fluent-locale-rs/pull/8/files to unic-langid.

It's a WIP patch, but it works and passes all tests. I think there are couple more optimizations possible and I'd also like to get feedback from the author on my attempt to separate try_new from new_unchecked.

Performance looks good, except of the unchecked:

     Running /Users/zbraniecki/projects/unic-locale/target/release/deps/canonicalize-de07a657eb9bbd6d
Gnuplot not found, disabling plotting
langid_canonicalize     time:   [3.1782 us 3.1955 us 3.2135 us]                                 
                        change: [-38.116% -37.401% -36.763%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
  1 (1.00%) low mild
  1 (1.00%) high mild

Gnuplot not found, disabling plotting
     Running /Users/zbraniecki/projects/unic-locale/target/release/deps/langid-c1dada57b49294d8
Gnuplot not found, disabling plotting
language_identifier_from_str                                                                             
                        time:   [512.60 ns 513.77 ns 515.01 ns]
                        change: [-84.009% -83.919% -83.827%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
  5 (5.00%) low mild
  4 (4.00%) high mild

language_identifier_from_parts                                                                            
                        time:   [293.03 ns 294.23 ns 295.60 ns]
                        change: [-89.997% -89.930% -89.862%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 11 outliers among 100 measurements (11.00%)
  3 (3.00%) low mild
  5 (5.00%) high mild
  3 (3.00%) high severe

language_identifier_from_parts_unchecked                                                                            
                        time:   [220.00 ns 221.03 ns 222.26 ns]
                        change: [+49.424% +50.201% +50.996%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 6 outliers among 100 measurements (6.00%)
  1 (1.00%) low severe
  1 (1.00%) low mild
  2 (2.00%) high mild
  2 (2.00%) high severe

Gnuplot not found, disabling plotting
     Running /Users/zbraniecki/projects/unic-locale/target/release/deps/parser-9a3dedd695250831
Gnuplot not found, disabling plotting
language_identifier_parser                                                                            
                        time:   [432.51 ns 433.59 ns 434.83 ns]
                        change: [-85.793% -85.707% -85.617%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
  1 (1.00%) low mild
  5 (5.00%) high mild
  3 (3.00%) high severe

language_identifier_parser_casing                                                                            
                        time:   [432.55 ns 433.62 ns 434.78 ns]
                        change: [-85.559% -85.467% -85.376%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
  2 (2.00%) low mild
  6 (6.00%) high mild

Gnuplot not found, disabling plotting

This patch so far is backwards compatible which is super nice. But I'm now wondering if it should return TinyStr from get_* methods (which, I assume, would make the unchecked constructor cheaper/faster), but it would be a breaking change and would let people operate on TinyStr rather than str. Not sure if it's worth it...

zbraniecki · 2019-08-07T18:28:37Z

@raphlinus - hi! would you have time to take a look at this and verify if I'm doing the right thing?

I'm concerned about the unchecked perf and also about the Ord on TinyStr8.

raphlinus

Some comments inline.

Unchecked performance is an interesting question - the reason why it was good before is that you allowed Cow<'static, str> so there's basically no work to be done. Since these are static strings, there is a potential strategy, which is to use the integers directly. One thing to explore is whether aggressive inlining of the TinyStr constructor would let the compiler do this. If const fn gets more powerful, then it might be possible to write the code carefully to be const fn.

Another possibility to explore, if identifiers from static sources are important, is a macro.

raphlinus · 2019-08-08T00:35:16Z

unic-langid-impl/src/tinystr.rs

+    pub fn new_unchecked(text: &str) -> TinyStr8 {
+        unsafe {
+            let mut word: u64 = 0;
+            copy_nonoverlapping(text.as_ptr(), &mut word as *mut u64 as *mut u8, text.len());


I think it might be possible to do this without unsafe, using a [u8; 8] buffer and then u64::from_ne_bytes, which is present as of 1.32.

Hmmm, I'm trying to do that, but I failed to find a way to get [u8; 8] out of a &str. What API should I use?

nevermind, I'll actually go with dumping numbers and new_unchecked will take it back.

raphlinus · 2019-08-08T00:40:20Z

unic-langid-impl/src/tinystr.rs

+    fn cmp(&self, other: &TinyStr8) -> Ordering {
+        //XXX: Is that correct?
+        // I see that `nedis` < `macos` otherwise - see tests :(
+        other.0.cmp(&self.0)


I suspect what you want to do here is self.0.to_be().cmp(other.0.to_be()).

raphlinus · 2019-08-08T00:42:15Z

unic-langid-impl/src/tinystr.rs

+        assert!(TinyStr8::try_new("abcXYZ").unwrap().is_all_ascii_alpha());
+    }
+
+    #[test]


This definitely needs more cases: strings of different lengths, etc.

raphlinus · 2019-08-08T00:45:10Z

unic-langid-impl/src/tinystr.rs

+pub struct TinyStr4(NonZeroU32);
+
+impl TinyStr8 {
+    pub fn new_unchecked(text: &str) -> TinyStr8 {


This is unsafe as written, so should be pub unsafe fn.

zbraniecki · 2019-08-08T00:53:33Z

Another possibility to explore, if identifiers from static sources are important, is a macro.

This is exactly for a macro - see https://github.com/zbraniecki/unic-locale/blob/master/unic-langid-macros-impl/src/lib.rs#L12 and https://github.com/zbraniecki/unic-locale/blob/master/unic-langid-macros/examples/langid-macro.rs

I'd like to not make it slower if possible. I'm wondering if I could just have a method that returns the structure as TinyStr that can be passed to from_parts_unchecked just for the macro use.

raphlinus · 2019-08-08T01:02:50Z

You should be able to do this by having a TinyStr constructor that takes an integer, and having your macro compute the integer for the string. Then performance would be spectacular.

One thing to watch out for is endianness: I recommend the TinyStr constructor always take the string in little-endian, so do .from_le() to swap on a BE system. It's very difficult to test on big-endian systems, as they're rare (but do exist).

Also keep in mind, that constructor needs to be unsafe, because otherwise you can deref the resulting TinyStr and get an invalid string.

zbraniecki · 2019-08-08T23:59:22Z

I updated this patch to be based on #8. The unchecked performance is now on par with the previous solution! :)

zbraniecki · 2019-08-09T18:15:00Z

Per emilio recommendation I also switched variants from Vec<_> to Box<[_]>. I was hoping it'll unblock production of LanguageIdentifier in const fn, but unfortunately that's still blocked on, this time on Box::new.

I could try to bring ToTokens into TinyStr, but I'd like to avoid setting this as a dependency on tinystr crate so I think for now I'll give up on langid! working in static.

Manishearth · 2019-08-09T18:17:23Z

Wait, why do you need ToTokens on TinyStr? In the macro you can just expand to an invocation of new_unchecked, no?

zbraniecki · 2019-08-09T18:18:42Z

Wait, why do you need ToTokens on TinyStr? In the macro you can just expand to an invocation of new_unchecked, no?

Yeah, but then I need to produce:

    let variants: Vec<_> = variants
        .into_iter()
        .map(|v| quote!($crate::TinyStr8::new_unchecked(#v)))
        .collect();
    let variants = quote!(Box::new([#(#variants,)*]));

Am I wrong?

Manishearth · 2019-08-09T18:18:49Z

You'll need to expose an as_int() -> Option<u64> function on it and use it in the macro

Manishearth · 2019-08-09T18:19:23Z

Never mind there's already an Into impl

Manishearth · 2019-08-09T18:22:14Z

What's wrong with that approach?

zbraniecki · 2019-08-09T18:23:26Z

What's wrong with that approach?

When I try to do:

static LANGID: LanguageIdentifier = langid!("en-US")

I am getting an error:

error[E0015]: calls in statics are limited to constant functions, tuple structs and tuple variants
 --> unic-langid/examples/simple-langid.rs:6:37
  |
6 | static LANGID: LanguageIdentifier = langid!("en-US");

Manishearth · 2019-08-09T18:29:13Z

Ah. That could be fixed if we used SmallVec as well I guess. I guess you still can't allocate in const?

Manishearth · 2019-08-09T18:29:43Z

Anyway you can always put these in lazy_static

zbraniecki · 2019-08-09T18:30:30Z

Ah. That could be fixed if we used SmallVec as well I guess. I guess you still can't allocate in const?

Why would it be fixed? SmallVec constructor also is not const.

zbraniecki · 2019-08-09T18:32:40Z

@Manishearth - I think this is big enough to warrant a review on its own before I pile up more. The TinyStr crate is in review in a separate issue so this is only about the second commit.

Manishearth · 2019-08-09T18:32:45Z

We can add one for constructing from arrays :)

zbraniecki · 2019-08-09T18:33:25Z

yes pls :)

Manishearth · 2019-08-09T18:33:33Z

Is there a reason we're vendoring TinyStr? Is this the canonical location? May be worth maintaining in a separate repo

zbraniecki · 2019-08-09T18:34:23Z

Is there a reason we're vendoring TinyStr? Is this the canonical location? May be worth maintaining in a separate repo

No preference. If you think it's worth it, I can separate out.

Manishearth · 2019-08-09T20:15:26Z

That would be ideal, thanks 😄

Manishearth

I didn't review the tinystr code, but this looks good!

Manishearth · 2019-08-09T20:16:10Z

unic-langid-impl/src/lib.rs

-    script: Option<Cow<'static, str>>,
-    region: Option<Cow<'static, str>>,
-    variants: Vec<Cow<'static, str>>,
+    language: Option<TinyStr8>,


Comment saying that these are guaranteed to be <=4

zbraniecki force-pushed the tinystr branch 2 times, most recently from 53a46e5 to 60c3bc0 Compare August 7, 2019 18:27

zbraniecki mentioned this pull request Aug 7, 2019

Beginning of "tinystr" optimization projectfluent/fluent-langneg-rs#8

Closed

raphlinus reviewed Aug 8, 2019

View reviewed changes

zbraniecki mentioned this pull request Aug 8, 2019

Add a helper TinyStr crate #8

Closed

zbraniecki force-pushed the tinystr branch from 60c3bc0 to c9c620b Compare August 8, 2019 23:57

zbraniecki force-pushed the tinystr branch 4 times, most recently from 53b1a32 to 47f6efc Compare August 9, 2019 17:03

zbraniecki marked this pull request as ready for review August 9, 2019 17:04

zbraniecki force-pushed the tinystr branch from 47f6efc to ab5cafa Compare August 9, 2019 18:13

zbraniecki force-pushed the tinystr branch from ab5cafa to 75e9eb0 Compare August 9, 2019 18:29

zbraniecki force-pushed the tinystr branch from 75e9eb0 to f439bc0 Compare August 9, 2019 18:31

zbraniecki requested a review from Manishearth August 9, 2019 18:31

zbraniecki force-pushed the tinystr branch from f439bc0 to fa23e70 Compare August 9, 2019 18:39

Manishearth approved these changes Aug 9, 2019

View reviewed changes

zbraniecki force-pushed the tinystr branch 3 times, most recently from 81c1bef to 80462ce Compare August 9, 2019 23:38

Migrate unic-langid to use TinyStr

378718b

zbraniecki force-pushed the tinystr branch from 80462ce to 378718b Compare August 10, 2019 04:07

zbraniecki merged commit 8860329 into master Aug 10, 2019

zbraniecki deleted the tinystr branch October 29, 2019 00:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to TinyStr #7

Switch to TinyStr #7

zbraniecki commented Aug 7, 2019

zbraniecki commented Aug 7, 2019

raphlinus left a comment

raphlinus Aug 8, 2019

zbraniecki Aug 8, 2019

zbraniecki Aug 8, 2019

raphlinus Aug 8, 2019

raphlinus Aug 8, 2019

raphlinus Aug 8, 2019

zbraniecki commented Aug 8, 2019

raphlinus commented Aug 8, 2019 •

edited

Loading

zbraniecki commented Aug 8, 2019

zbraniecki commented Aug 9, 2019

Manishearth commented Aug 9, 2019

zbraniecki commented Aug 9, 2019

Manishearth commented Aug 9, 2019

Manishearth commented Aug 9, 2019

Manishearth commented Aug 9, 2019

zbraniecki commented Aug 9, 2019

Manishearth commented Aug 9, 2019

Manishearth commented Aug 9, 2019

zbraniecki commented Aug 9, 2019

zbraniecki commented Aug 9, 2019

Manishearth commented Aug 9, 2019

zbraniecki commented Aug 9, 2019

Manishearth commented Aug 9, 2019

zbraniecki commented Aug 9, 2019

Manishearth commented Aug 9, 2019

Manishearth left a comment

Manishearth Aug 9, 2019

Switch to TinyStr #7

Switch to TinyStr #7

Conversation

zbraniecki commented Aug 7, 2019

zbraniecki commented Aug 7, 2019

raphlinus left a comment

Choose a reason for hiding this comment

raphlinus Aug 8, 2019

Choose a reason for hiding this comment

zbraniecki Aug 8, 2019

Choose a reason for hiding this comment

zbraniecki Aug 8, 2019

Choose a reason for hiding this comment

raphlinus Aug 8, 2019

Choose a reason for hiding this comment

raphlinus Aug 8, 2019

Choose a reason for hiding this comment

raphlinus Aug 8, 2019

Choose a reason for hiding this comment

zbraniecki commented Aug 8, 2019

raphlinus commented Aug 8, 2019 • edited Loading

zbraniecki commented Aug 8, 2019

zbraniecki commented Aug 9, 2019

Manishearth commented Aug 9, 2019

zbraniecki commented Aug 9, 2019

Manishearth commented Aug 9, 2019

Manishearth commented Aug 9, 2019

Manishearth commented Aug 9, 2019

zbraniecki commented Aug 9, 2019

Manishearth commented Aug 9, 2019

Manishearth commented Aug 9, 2019

zbraniecki commented Aug 9, 2019

zbraniecki commented Aug 9, 2019

Manishearth commented Aug 9, 2019

zbraniecki commented Aug 9, 2019

Manishearth commented Aug 9, 2019

zbraniecki commented Aug 9, 2019

Manishearth commented Aug 9, 2019

Manishearth left a comment

Choose a reason for hiding this comment

Manishearth Aug 9, 2019

Choose a reason for hiding this comment

raphlinus commented Aug 8, 2019 •

edited

Loading