projectfluent / fluent-rs

Rust implementation of Project Fluent
https://projectfluent.org
Apache License 2.0
1.07k stars 96 forks source link

Switch FluentBundle::format_pattern to return errors. #126

Closed zbraniecki closed 3 years ago

zbraniecki commented 5 years ago

Fixes #125

zbraniecki commented 5 years ago

The performance impact is not-negligible at this point.

Zibis-MacBook-Pro:fluent-bundle (master)$ git co pattern-errors
Switched to branch 'pattern-errors'
Zibis-MacBook-Pro:fluent-bundle (pattern-errors)$ cargo bench
   Compiling fluent-bundle v0.6.0 (/Users/zbraniecki/projects/fluent/fluent-rs/fluent-bundle)
    Finished release [optimized] target(s) in 3.17s
     Running /Users/zbraniecki/projects/fluent/fluent-rs/target/release/deps/fluent_bundle-7bfff413cd359b88

running 1 test
test types::tests::value_from_copy_ref ... ignored

test result: ok. 0 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out

     Running /Users/zbraniecki/projects/fluent/fluent-rs/target/release/deps/resolver-90a8ad21eb4c2c9d
Gnuplot not found, disabling plotting
resolve/"simple"        time:   [7.7656 us 7.8012 us 7.8413 us]                              
                        change: [+27.844% +28.822% +29.753%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
  3 (3.00%) high mild
  1 (1.00%) high severe
resolve/"preferences"   time:   [95.874 us 96.386 us 96.992 us]                                  
                        change: [+10.112% +11.263% +12.481%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 6 outliers among 100 measurements (6.00%)
  2 (2.00%) high mild
  4 (4.00%) high severe
resolve/"menubar"       time:   [25.244 us 25.351 us 25.455 us]                               
                        change: [+13.639% +14.355% +15.187%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 5 outliers among 100 measurements (5.00%)
  1 (1.00%) high mild
  4 (4.00%) high severe
resolve/"unescape"      time:   [1.3537 us 1.3613 us 1.3699 us]                                
                        change: [+7.6681% +8.7710% +9.8918%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
  3 (3.00%) high mild
  1 (1.00%) high severe

Gnuplot not found, disabling plotting

@Manishearth - do you have any thoughts on which API is more canonical for Rust and if there's anything we can do about the performance cost here?

zbraniecki commented 5 years ago

I think I start to like the &mut errors more as I think about it. It is easier to reason about, makes it explicit that you need to care about errors (I noticed that the same pattern happens in Gecko C++ when we work with fallible methods) rather than just let (value, _) = ... and is faster for consumers since it reduces allocations.

I can play more with that, but the proposal to move to returning errors feels less canonical now and I'm not sure if there's a good reason to push for it.

@stasm - do you think we still should explore it?

zbraniecki commented 5 years ago

With a couple guards around I got a comparable performance to master!

Zibis-MacBook-Pro:fluent-bundle (pattern-errors)$ cargo bench
   Compiling fluent-bundle v0.6.0 (/Users/zbraniecki/projects/fluent/fluent-rs/fluent-bundle)
    Finished release [optimized] target(s) in 3.36s
     Running /Users/zbraniecki/projects/fluent/fluent-rs/target/release/deps/fluent_bundle-ac2963ec4de9805d

running 1 test
test types::tests::value_from_copy_ref ... ignored

test result: ok. 0 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out

     Running /Users/zbraniecki/projects/fluent/fluent-rs/target/release/deps/resolver-1ddcc31ef1ff5b5d
Gnuplot not found, disabling plotting
resolve/"simple"        time:   [6.1629 us 6.1829 us 6.2044 us]                              
                        change: [-2.5953% -1.9027% -1.1887%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
  4 (4.00%) high mild
  3 (3.00%) high severe
resolve/"preferences"   time:   [90.768 us 91.164 us 91.571 us]                                  
                        change: [+0.4219% +1.0667% +1.7239%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 3 outliers among 100 measurements (3.00%)
  1 (1.00%) high mild
  2 (2.00%) high severe
resolve/"menubar"       time:   [22.983 us 23.071 us 23.166 us]                               
                        change: [+0.5774% +1.1035% +1.6015%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 4 outliers among 100 measurements (4.00%)
  4 (4.00%) high mild
resolve/"unescape"      time:   [1.2885 us 1.2935 us 1.2985 us]                                
                        change: [+0.1344% +1.0848% +1.8803%] (p = 0.01 < 0.05)
                        Change within noise threshold.
Found 4 outliers among 100 measurements (4.00%)
  4 (4.00%) high mild

Gnuplot not found, disabling plotting

I'm not super happy about the guards needed (I would have hoped that Rust will optimize it away), but glad to see that performance can be taken out of equation as a factor in this decision.

zbraniecki commented 5 years ago

I talked to Stas about it and he okayed postponing this discussion till 0.8.