tech-debt: Context lib #4060
tech-debt: Context lib #4060
Conversation
CodSpeed Performance ReportMerging #4060 will not alter performancesComparing Summary
|
| pub fn concurrent(self) -> Arc<Mutex<Context>> { | ||
| Arc::new(Mutex::new(self)) | ||
| } |
There was a problem hiding this comment.
I like that the Arc<Mutex<..>> overhead is optional :)
|
|
||
| #[derive(Debug, Default)] | ||
| pub struct Context { | ||
| store: HashMap<(String, TypeId), Box<dyn Any>>, |
There was a problem hiding this comment.
Could we adopt something different than a String as a key type?
I'd strive to avoid too many memory allocations just to retrieve dynamically populated values from Context. Could the key be represented a closed set type, like a numeric enum, whose static list is append-only (similar to the introspection warning codes list we had some months ago)?
E.g.:
enum ContextKey {
TRACING,
PRISMA_QUERY,
...
}and
#[derive(Debug, Default)]
pub struct Context {
store: HashMap<(ContextKey, TypeId), Box<dyn Any>>,
}There was a problem hiding this comment.
One can also try to extend this idea to TypeId as well, but I think that can happen in a future query and only if we see a noticeable slowdown in benchmarked performance.
There was a problem hiding this comment.
No we can't sorry, if we had an enum then the enum has to be known by the context, thus making it coupled to it (which will be a very bad design decision) On top of that if this is passed down as owned value it won't be cloned so there would only be one allocation per key (in case it's not wrapped in an Arc)
What I will do in case needed, is to make it receive a &'static str as a key, as the users of context will identify the keys by name, but rather not prematurely optimize anything until I see how this is applied when refactoring (I have one in progress)
There was a problem hiding this comment.
I see. I was recommending intentionally coupling Context with its available ContextKeys for the sake of avoiding String allocations, which have already bit us several times in the past: we risk using Strings now just to replace them later. Considering this is a library for the Query Engine and not a general purpose crate, I’d find the coupling acceptable
Problem
All the way down the stack, from the query engine entry point down to quiant, we pass a trace id for issuing telemetry traces. This is a very specific cross-cutting concern, the moment we need more things to be passed in, we will need more and more parameters to functions being called.
Because most of the behaviors down the stack are dynamic (i.e. a trait implementation with specific methods provides the behavior, rather than a struct holding the state) it’s each function in the trait that needs to be parameterized and changed in all the implementations. Consider this commit for instance (from a hackathon project), that passes down a prisma query, only for a subset of the read operations, it had to change more than 600 lines of code:
da24144
This lack of a mechanism to pass down a query context down the stack was also the reason why telemetry capturing was so complex. If it were in place it would have been much easier to implement that feature.
Goals