Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tech-debt: Context lib #4060

Merged
merged 1 commit into from
Jul 3, 2023
Merged

tech-debt: Context lib #4060

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions libs/context/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
[package]
name = "context"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
87 changes: 87 additions & 0 deletions libs/context/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
use std::{
any::{Any, TypeId},
collections::HashMap,
sync::{Arc, Mutex},
};

#[derive(Debug, Default)]
pub struct Context {
store: HashMap<(String, TypeId), Box<dyn Any>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>>,
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@miguelff miguelff Jul 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@jkomyno jkomyno Jul 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

impl Context {
pub fn concurrent(self) -> Arc<Mutex<Context>> {
Arc::new(Mutex::new(self))
}
Comment on lines +13 to +15
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that the Arc<Mutex<..>> overhead is optional :)


pub fn insert<T: Any>(&mut self, key: &str, value: T) {
self.store.insert((key.to_owned(), TypeId::of::<T>()), Box::new(value));
}

pub fn get<T: Any>(&self, key: &str) -> Option<&T> {
self.store
.get(&(key.to_owned(), TypeId::of::<T>()))
.map(|v| v.downcast_ref::<T>().unwrap())
}

pub fn get_mut<T: Any>(&mut self, key: &str) -> Option<&mut T> {
self.store
.get_mut(&(key.to_owned(), TypeId::of::<T>()))
.map(|v| v.downcast_mut::<T>().unwrap())
}

pub fn remove<T: Any>(&mut self, key: &str) -> Option<T> {
self.store
.remove(&(key.to_owned(), TypeId::of::<T>()))
.map(|v| *v.downcast::<T>().unwrap())
}
}

#[cfg(test)]
mod tests {
use super::Context;

#[test]
fn set_and_retrieve() {
let mut ctx: Context = Context::default();
ctx.insert("foo", 42 as u32);

let val: u32 = *ctx.get("foo").unwrap();
assert_eq!(val, 42 as u32)
}

#[test]
fn concurrent() {
let mut ctx: Context = Context::default();
ctx.insert("foo", 42 as u32);

assert_eq!(42 as u32, *ctx.get::<u32>("foo").unwrap());
assert_eq!(None, ctx.get::<u32>("bar"));

let safe_context = ctx.concurrent();
let mut ctx = safe_context.lock().unwrap();
ctx.insert("bar", 32 as u32);
assert_eq!(32 as u32, *ctx.get::<u32>("bar").unwrap());
assert_eq!(42 as u32, *ctx.get::<u32>("foo").unwrap());
}

#[test]
fn get_mut() {
let mut ctx: Context = Context::default();
ctx.insert("foo", 42 as u32);

let val: &mut u32 = ctx.get_mut("foo").unwrap();
*val = 32 as u32;
assert_eq!(32 as u32, *ctx.get::<u32>("foo").unwrap());
}

#[test]
fn remove() {
let mut ctx: Context = Context::default();
ctx.insert("foo", 42 as u32);

let val: u32 = ctx.remove("foo").unwrap();
assert_eq!(42 as u32, val);
assert_eq!(None, ctx.get::<u32>("foo"));
}
}
Loading