Skip to content

Commit ad8b821

Browse files
chore(turbo_json): remove exterior mutability from loader (#10066)
### Description `TurboJsonLoader::load` only took a mutable self reference because we were using `HashMap` to cache the results. This PR changes the data type we use to cache results and removes the exterior mutability. We move to use a `FixedMap` which has the following properties: - Only works with the keys provided at construction - Append only - Thread safe I went with this just because the implementation is straightforward and matches our use case since we know all of the possible places a `turbo.json` will be before we start loading them. Future work can be reworking boundaries code to no longer eagerly load all of the `turbo.json`s at the start of execution. ### Testing Instructions Existing test suite. Additional unit tests for `FixedMap` --------- Co-authored-by: Nicholas Yang <[email protected]>
1 parent cf03598 commit ad8b821

File tree

10 files changed

+211
-63
lines changed

10 files changed

+211
-63
lines changed

Cargo.lock

+5
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

+1
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ turborepo-cache = { path = "crates/turborepo-cache" }
5757
turborepo-ci = { path = "crates/turborepo-ci" }
5858
turborepo-env = { path = "crates/turborepo-env" }
5959
turborepo-errors = { path = "crates/turborepo-errors" }
60+
turborepo-fixed-map = { path = "crates/turborepo-fixed-map" }
6061
turborepo-fs = { path = "crates/turborepo-fs" }
6162
turborepo-lib = { path = "crates/turborepo-lib", default-features = false }
6263
turborepo-lockfiles = { path = "crates/turborepo-lockfiles" }

crates/turborepo-fixed-map/Cargo.toml

+10
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
[package]
2+
name = "turborepo-fixed-map"
3+
version = "0.1.0"
4+
edition = "2021"
5+
license = "MIT"
6+
7+
[dependencies]
8+
9+
[lints]
10+
workspace = true

crates/turborepo-fixed-map/src/lib.rs

+120
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
#![deny(clippy::all)]
2+
3+
use std::sync::OnceLock;
4+
5+
/// An error indicating that the key wasn't given to the constructor.
6+
#[derive(Debug)]
7+
pub struct UnknownKey;
8+
9+
/// A FixedMap is created with every key known at the start and cannot have
10+
/// value removed or written over.
11+
#[derive(Debug)]
12+
pub struct FixedMap<K, V> {
13+
inner: Vec<(K, OnceLock<V>)>,
14+
}
15+
16+
impl<K: Ord, V> FixedMap<K, V> {
17+
pub fn new(keys: impl Iterator<Item = K>) -> Self {
18+
let mut inner = keys.map(|key| (key, OnceLock::new())).collect::<Vec<_>>();
19+
inner.sort_by(|(k1, _), (k2, _)| k1.cmp(k2));
20+
Self { inner }
21+
}
22+
23+
/// Get a value for a key.
24+
///
25+
/// Returns `None` if key hasn't had a value inserted yet.
26+
pub fn get(&self, key: &K) -> Result<Option<&V>, UnknownKey> {
27+
let item_index = self
28+
.inner
29+
.binary_search_by(|(k, _)| k.cmp(key))
30+
.map_err(|_| UnknownKey)?;
31+
let (_, value) = &self.inner[item_index];
32+
Ok(value.get())
33+
}
34+
35+
/// Insert a value for a key.
36+
///
37+
/// There is no guarentee that the provided value will be the one returned.
38+
pub fn insert(&self, key: &K, value: V) -> Result<&V, UnknownKey> {
39+
let item_index = self
40+
.inner
41+
.binary_search_by(|(k, _)| k.cmp(key))
42+
.map_err(|_| UnknownKey)?;
43+
let (_, value_slot) = &self.inner[item_index];
44+
// We do not care about if this set was successful or if another call won out.
45+
// The end result is that we have a value for the key.
46+
let _ = value_slot.set(value);
47+
Ok(value_slot
48+
.get()
49+
.expect("OnceLock::set will always result in a value being present in the lock"))
50+
}
51+
}
52+
53+
impl<K: Clone, V: Clone> Clone for FixedMap<K, V> {
54+
fn clone(&self) -> Self {
55+
Self {
56+
inner: self.inner.clone(),
57+
}
58+
}
59+
}
60+
61+
impl<K: Ord, V> FromIterator<(K, Option<V>)> for FixedMap<K, V> {
62+
fn from_iter<T: IntoIterator<Item = (K, Option<V>)>>(iter: T) -> Self {
63+
let mut inner = iter
64+
.into_iter()
65+
.map(|(key, value)| {
66+
let value_slot = OnceLock::new();
67+
if let Some(value) = value {
68+
value_slot
69+
.set(value)
70+
.map_err(|_| ())
71+
.expect("nobody else has access to this lock yet");
72+
}
73+
(key, value_slot)
74+
})
75+
.collect::<Vec<_>>();
76+
inner.sort_by(|(k1, _), (k2, _)| k1.cmp(k2));
77+
Self { inner }
78+
}
79+
}
80+
81+
#[cfg(test)]
82+
mod test {
83+
use super::*;
84+
85+
#[test]
86+
fn test_get() {
87+
let map: FixedMap<i32, ()> = FixedMap::new([3, 2, 1].iter().copied());
88+
assert_eq!(map.get(&2).unwrap(), None);
89+
assert!(map.get(&4).is_err());
90+
}
91+
92+
#[test]
93+
fn test_set() {
94+
let map: FixedMap<i32, bool> = FixedMap::new([3, 2, 1].iter().copied());
95+
assert!(map.insert(&4, true).is_err());
96+
assert_eq!(map.insert(&2, true).unwrap(), &true);
97+
assert_eq!(map.insert(&2, false).unwrap(), &true);
98+
}
99+
100+
#[test]
101+
fn test_contention() {
102+
let map: FixedMap<i32, bool> = FixedMap::new([3, 2, 1].iter().copied());
103+
let results: Vec<_> = std::thread::scope(|scope| {
104+
let mut handles = vec![];
105+
let map = &map;
106+
for i in 0..16 {
107+
let is_even = i % 2 == 0;
108+
handles.push(scope.spawn(move || {
109+
let val = map.insert(&1, is_even).unwrap();
110+
(val, *val)
111+
}));
112+
}
113+
handles.into_iter().map(|h| h.join().unwrap()).collect()
114+
});
115+
116+
for (val_ref, val) in results {
117+
assert_eq!(*val_ref, val, "all values should remain the same");
118+
}
119+
}
120+
}

crates/turborepo-lib/Cargo.toml

+1
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,7 @@ turborepo-dirs = { path = "../turborepo-dirs" }
133133
turborepo-env = { workspace = true }
134134
turborepo-errors = { workspace = true }
135135
turborepo-filewatch = { path = "../turborepo-filewatch" }
136+
turborepo-fixed-map = { workspace = true }
136137
turborepo-fs = { path = "../turborepo-fs" }
137138
turborepo-graph-utils = { path = "../turborepo-graph-utils" }
138139
turborepo-lockfiles = { workspace = true }

crates/turborepo-lib/src/boundaries/tags.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ impl From<Permissions> for ProcessedPermissions {
5151
impl Run {
5252
pub(crate) fn get_package_tags(&self) -> HashMap<PackageName, Spanned<Vec<Spanned<String>>>> {
5353
let mut package_tags = HashMap::new();
54-
let mut turbo_json_loader = self.turbo_json_loader();
54+
let turbo_json_loader = self.turbo_json_loader();
5555
for (package, _) in self.pkg_dep_graph().packages() {
5656
if let Ok(TurboJson {
5757
tags: Some(tags),

0 commit comments

Comments
 (0)