refactoring Rc->Arc#656
Conversation
|
I agree that this needs to be addressed. However: We almost certainly don't want to do all of these — e.g., query parser internals shouldn't escape across thread boundaries, so they don't need to be This is also an opportunity to do better encapsulation; we shouldn't be leaking that strings and keywords are More broadly, I'm not convinced that directly switching We use
The reason we introduced My suspicion is that we need to introduce some kind of context against which |
| // in order to dedupe. We'll add these to the regular argument vector later. | ||
| byte_args: HashMap<Vec<u8>, String>, // From value to argument name. | ||
| string_args: HashMap<Rc<String>, String>, // From value to argument name. | ||
| string_args: HashMap<Arc<String>, String>, // From value to argument name. |
There was a problem hiding this comment.
I see no reason why this would need to change. Did you just replace every Rc in the project, or did you make the changes necessary to pass some particular test?
There was a problem hiding this comment.
Some changes to Arc simplified the refactoring as most code with clone or cloned could remain the same instead of implementing some convert from Arc->Rc which would require to clone the inner value, so just cloning Arc potentially is cheaper then cloning the inner value into a new Rc.
|
I think I managed to boil this PR down to a one-line change in I'd like to get a little better understanding of the implications before I make that one-line change, and some other work needs to happen before Mentat is truly usable across threads (#660, for example). |
) @mmacedoeu did a good deal of work to show that Arc instead of Rc wasn't too difficult in mozilla#656, and @rnewman pushed the refactoring across the line in mozilla#659. However, we didn't flip the switch at that time. For mozilla#673, we'd like to include TypedValue instances in errors, and with error-chain (and failure) error types need to be 'Sync + 'Send, so we need Arc. This builds on mozilla#659 and should also finish mozilla#656.
) r=rnewman @mmacedoeu did a good deal of work to show that Arc instead of Rc wasn't too difficult in #656, and @rnewman pushed the refactoring across the line in #659. However, we didn't flip the switch at that time. For #673, we'd like to include TypedValue instances in errors, and with error-chain (and failure) error types need to be 'Sync + 'Send, so we need Arc. This builds on #659 and should also finish #656.
|
Fixed in #678. |
2 participants
Refactoring to Arc will get Send and ability for use on multi-threaded environments like webapps