Skip to content

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

Merged
zbraniecki merged 1 commit intoprojectfluent:masterfrom
zbraniecki:rwlock
Feb 16, 2020
Merged

Switch RefCell to Mutex to fix the regression allowing FluentBundle to Send+Sync#158
zbraniecki merged 1 commit intoprojectfluent:masterfrom
zbraniecki:rwlock

Conversation

@zbraniecki
Copy link
Copy Markdown
Collaborator

This should unblock Manishearth/handlebars-fluent#4 and remove the regression that is likely to also prevent Amethyst from upgrading.

@zbraniecki
Copy link
Copy Markdown
Collaborator Author

@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
Copy link
Copy Markdown
Collaborator

@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
Copy link
Copy Markdown
Collaborator

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
Copy link
Copy Markdown
Collaborator Author

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
Copy link
Copy Markdown
Collaborator

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

@zbraniecki
Copy link
Copy Markdown
Collaborator Author

Can I get r+ then? :)

@Manishearth
Copy link
Copy Markdown
Collaborator

Make it a mutex and r=me

@Manishearth
Copy link
Copy Markdown
Collaborator

(perhaps leave a comment with a followup issue linked)

@zbraniecki zbraniecki changed the title Switch RefCell to RwLock to fix the regression allowing FluentBundle to Send+Sync Switch RefCell to Mutex to fix the regression allowing FluentBundle to Send+Sync Feb 14, 2020
@zbraniecki
Copy link
Copy Markdown
Collaborator Author

done.

@zbraniecki zbraniecki merged commit b3fda98 into projectfluent:master Feb 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants