projectfluent / fluent-rs

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

Evaluate Send/Sync of FluentBundle #159

Closed zbraniecki closed 4 years ago

zbraniecki commented 4 years ago

In https://github.com/projectfluent/fluent-rs/pull/158 we switched to use Mutex to allow FluentBundle to work in multithreaded scenarios.

This may or may not be the right choice and we should evaluate alternatives.

Manishearth commented 4 years ago

One improvement that can be made is to have the memoizer use a read lock most of the time, and a write lock only when adding new entries (rare). This would need us to add an immutable get(&self) -> Option<..> method to the memoizer.

Aside from that, potential API designs:

zbraniecki commented 4 years ago

In https://phabricator.services.mozilla.com/D57402 @emilio suggest to switch from Mutex to parking_lot::RwLock - https://amanieu.github.io/parking_lot/parking_lot/struct.RwLock.html and use upgradable_read - https://searchfox.org/mozilla-central/source/servo/components/style/rule_tree/mod.rs#1255

@Manishearth would you recommend such change? I personally would prefer to avoid having to add a dependency on parking_lot so std's RwLock and desugar the call into an immutable get+ mutable set to only require lock when needed, but I don't have enough experience with Send+Sync code to evaluate all major pros and cons of such solution.

Manishearth commented 4 years ago

I would recommend this. parking_lot is a good crate, and Firefox already depends on it.

I personally would prefer to avoid having to add a dependency on parking_lot so std's RwLock and desugar the call into an immutable get+ mutable set to only require lock when needed

I mean, this is fine too, but it still involves locking multiple times.

It might be worth making threadsafety an opt-in optional feature; people who need it can enable it and everyone else can continue with the existing system with less contention.

zbraniecki commented 4 years ago

This is now covered by #165.