projectfluent / fluent-rs

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

Introduce a concurrent version of FluentBundle #163

Closed zbraniecki closed 4 years ago

zbraniecki commented 4 years ago

We now have three elements of the FluentBundle that may or may not have to be concurrent:

Until the last one showed up, the problem wasn't very noticeable because in most scenarios both transform and formatter will stay None and when you do set it, you can just make your Fn Send+Sync.

With the introduction of IntlMemoizer this becomes a real chore because now we have a Mutex in the heart of the FluentBundle and a lock on every number formatting operation or plural rules selection.

One way out of that is to follow what https://docs.rs/type-map/0.3.0/type_map/ did and introduce fluent_bundle::concurrent::FluentBundle which would be exactly the same as the regular FluentBundle except that it would require Fns to be Send + Sync and use Mutex for intls.

The regular FluentBundle then would remove those requirements and use RefCell for IntlLangMemoizer.

Then, customers like Gecko would use the regular FluentBundle while amethyst and handlebars_fluent would use the concurrent version.

@emilio, @Manishearth - does it look like a sound solution to you?

zbraniecki commented 4 years ago

If so, is there a good best-practice on how to introduce such two versions of the same API without having to duplicate the code? My first idea would be to introduce sth like:

pub struct FluentBundleBase<R, I, F> {
    pub(crate) resources: Vec<R>,
    pub(crate) intls: I<IntlLangMemoizer>,
    pub(crate) transform: Option<Box<F<dyn Fn(&str) -> Cow<str>>>>,
    pub(crate) formatter:
        Option<Box<F<dyn Fn(&FluentValue, &I<IntlLangMemoizer>) -> Option<String>>>>,
}

type FluentFn<F> = F;
type FluentBundle = FluentBundleBase<R, RefCell, FluentFn>;

mod concurrent {
  type FluentFn<F> = F + Send + Sync;
  type FluentBundle = FluentBundleBase<R, Mutex, FluentFn>;
}

Is that the path worth exploring or is there a better way?

zbraniecki commented 4 years ago

fixed in 0.11!