aboodman
commented
4 years ago I realize this seems like overkill for this issue since these are small strings. I think of it more as (a) "let's not establish a bad pattern, if we can help it", and (b) Rust learning experience that can be applied later to bigger problems. So thanks for looking into it seriously.
2) seems perfect to me. Rc::clone() doesn't allocate: https://doc.rust-lang.org/beta/src/alloc/rc.rs.html#1184. Although clone() frequently means "expensive", it really seems to be more about whether the type exhibits value/copy semantics. ref-counted pointers shouldn't be copied everywhere willy-nilly, even though they are cheap. Thus the way to copy them is via clone so that it's more explicit.
As an aside, I was curious if (3) was what Rust already does since the strategy of using the stack for short strings is common in other languages. But no, a Rust string is just a Vec, and a Vec is just a slice. I admire the dedication to purity :).
Re: https://github.com/rocicorp/repc/pull/187#pullrequestreview-491826587
I spent several hours trying to convert LogContext to something we could pass around by ref. I made
contextaRefCell<String>for inner mutability and this enabled us to pass the LogContext by reference while still being able to add context. However there are some downsides: all the stores and transactions need another lifetime parameter, there are multiple places where we cannot pass by reference and so have to clone a bunch anyway (spawn_local, idb callbacks, etc), and we don't get much benefit of top level calls seeing context added by lower levels bc we have to clone at the top level dispatch. After a few hours of fighting with it I'm pausing on that and hoping there is a better way.My next thought is to make the logging context itself an
Rc<RefCell<String>>. Rc models what we need for decoupled async futures perfectly and enables all holders of a logging context to share the same mutable context, so added context at a lower level is seen by all holders of the rc. However it still allocates a new Rc each time we pass it to a function because we have to clone it. This is better than what we have now in the sense that we'd only be allocating a pointer and not a String which is a pointer plus a buffer of bytes. But we're still allocating (though I would point out that any reasonable allocator is likely to have free list of pointer-sized memory units lying around at all times so it should not be expensive).Another thought was to make LogContext Copy. I was thinking something like
struct { context: [u8; 256] }. Having a limited length doesn't seem like a problem. This version doesn't have the feature of higher level calls benefitting from lower level context that is added :(Your ideas -- got any?
I am hoping for something simpler than 1 and better than 3. I like 2 because Rcs model the problem perfectly -- you can pass them anywhere -- and the logging context accumulates context from all holders. My sense is to do 2 and ~tell~ ask anyone who worries about the allocations to show me a profile that demonstrates the runtime cost is not worth the very large benefit.