search

projectfluent / fluent-rs

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

Switch RefCell to Mutex to fix the regression allowing FluentBundle to Send+Sync #158

Closed zbraniecki closed 4 years ago

zbraniecki commented 4 years ago

This should unblock https://github.com/Manishearth/handlebars-fluent/pull/4 and remove the regression that is likely to also prevent Amethyst from upgrading.

zbraniecki commented 4 years ago

@Manishearth - so, performance wise, microbenchmarks that we have show no sign of slowdown from this. What worries me a bit is that this requires yet another .expect which I don't know how likely is to be fallible in production.

I'm very unfamiliar with the whole thread management and send/sync so I'm not sure how reasonable it is to even expect intl memoizer to be accessible across threads. I'd be ok if memoizer was per-thread too, but I would like to minimize additional complexity to the API right now.

Does this patch look reasonable to you? Is the .expect in the FluentValue.matches ever going to be an actual issue or is it just in theory?

Manishearth commented 4 years ago

@Manishearth - so, performance wise, microbenchmarks that we have show no sign of slowdown from this.

It's only going to matter in multithreaded scenarios, it shouldn't slow down single threaded scenarios, but it can affect multithreaded scenarios where you're using the bundle on just one thread.

Manishearth commented 4 years ago

The expect is for lock poisoning: if one thread panics while holding onto the lock, other threads that try to acquire the lock will also panic.

What I'm more worried about is that we never use the read lock here. Ideally we should acquire the read lock and only use the write lock when you need to populate a cache entry. But that's tricky to write.

Either we should just use a mutex (which is going to slow down multithreaded use cases), or manually handle the caching a bit more. Sounds like work.

zbraniecki commented 4 years ago

Either we should just use a mutex (which is going to slow down multithreaded use cases), or manually handle the caching a bit more. Sounds like work.

Is there any way for us to unblock 0.10 regression fix, and continue working on a better solution for 0.11? Is this patch acceptable short-term, or would you recommend against merging it (in particular, into Gecko - where we don't use it multithreded)?

Manishearth commented 4 years ago

I think it's fine short term. It might even be fine long term, I'm just wary.

zbraniecki commented 4 years ago

Can I get r+ then? :)

Manishearth commented 4 years ago

Make it a mutex and r=me

Manishearth commented 4 years ago

(perhaps leave a comment with a followup issue linked)

zbraniecki commented 4 years ago

done.