projectfluent / fluent-rs

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

Intl memoizer use #150

Closed zbraniecki closed 4 years ago

zbraniecki commented 4 years ago

@Manishearth - here's how I'd use the memoizer. One challenge I see is that there's no easy way to have a Default if I need to allocate an IntlMemoizer externally.

I'm wondering if it would be possible to have FluentBundle either take an external one or allocate its own if not provided, but I'm not sure how to design such a generic.

Manishearth commented 4 years ago

Seems fine. I'd write a FluentBundle::new_with_memoizer function

zbraniecki commented 4 years ago

Hmm, continuing the process. Trying to hook one into another via a generic type to allow for construction of a FluentBundle that owns its IntlLangMemoizer vs one that borrows it.

Currently, I'm trying to get the bench to work, but when I do:

            let lang = langid!("en");
            let lids = &[lang.clone()];
            let mut memoizer = IntlMemoizer::default();
            let mut bundle = FluentBundle::new(lids, memoizer.get_for_lang(lang));

I'm getting:

error[E0277]: the trait bound `std::rc::Rc<std::cell::RefCell<intl_memoizer::IntlLangMemoizer>>: std::borrow::BorrowMut<intl_memoizer::IntlLangMemoizer>` is not satisfied
  --> fluent-bundle/benches/resolver.rs:90:54
   |
90 |             let mut bundle = FluentBundle::new(lids, memoizer.get_for_lang(lang));
   |                                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::borrow::BorrowMut<intl_memoizer::IntlLangMemoizer>` is not implemented for `std::rc::Rc<std::cell::RefCell<intl_memoizer::IntlLangMemoizer>>`
   |
   = note: required by `fluent_bundle::bundle::FluentBundle::<R, I>::new`
zbraniecki commented 4 years ago

I think I'm messing up the Rc<RefCell<IntlLangMemoizer>> vs RefCell<BorrowMut<IntlLangMemoizer>> relationship. :(

zbraniecki commented 4 years ago

Got it to work! Perf looks good I think:

▶ cargo bench
   Compiling fluent-bundle v0.9.0 (/projects/fluent-rs/fluent-bundle)
    Finished release [optimized] target(s) in 2.26s
     Running /projects/fluent-rs/target/release/deps/fluent_bundle-32f2072b4d2fe234

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 /projects/fluent-rs/target/release/deps/resolver-153afaede1425209
Gnuplot not found, disabling plotting
resolve/"simple"        time:   [5.2540 us 5.2693 us 5.2878 us]                              
                        change: [-0.6686% +0.3376% +1.0431%] (p = 0.51 > 0.05)
                        No change in performance detected.
Found 7 outliers among 100 measurements (7.00%)
  2 (2.00%) low mild
  3 (3.00%) high mild
  2 (2.00%) high severe
resolve/"preferences"   time:   [47.641 us 47.726 us 47.818 us]                                   
                        change: [-4.8989% -4.5535% -4.2343%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  2 (2.00%) high mild
  1 (1.00%) high severe
resolve/"menubar"       time:   [13.357 us 13.379 us 13.405 us]                               
                        change: [-6.9899% -6.6010% -6.2043%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
  1 (1.00%) low mild
  5 (5.00%) high mild
  2 (2.00%) high severe
resolve/"unescape"      time:   [673.75 ns 675.38 ns 677.34 ns]                                
                        change: [-2.4000% -2.0749% -1.7354%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 10 outliers among 100 measurements (10.00%)
  3 (3.00%) low mild
  3 (3.00%) high mild
  4 (4.00%) high severe

Gnuplot not found, disabling plotting
zbraniecki commented 4 years ago

I removed the shared memoizer constructor for now. Will revisit later.