From 2a2cc980133a1b7368c70dada86ae83f1b577017 Mon Sep 17 00:00:00 2001 From: Shahar Shalev Date: Wed, 14 Aug 2024 16:32:01 +0300 Subject: [PATCH 1/3] fix: consistent colors for packages --- crates/turborepo-ui/src/color_selector.rs | 43 +++++++++-------------- 1 file changed, 17 insertions(+), 26 deletions(-) diff --git a/crates/turborepo-ui/src/color_selector.rs b/crates/turborepo-ui/src/color_selector.rs index b2db3c6e00c95..3b9207c7c3269 100644 --- a/crates/turborepo-ui/src/color_selector.rs +++ b/crates/turborepo-ui/src/color_selector.rs @@ -1,21 +1,20 @@ use std::{ collections::HashMap, + hash::{DefaultHasher, Hash, Hasher}, sync::{Arc, OnceLock, RwLock}, + u8, }; use console::{Style, StyledObject}; -static COLORS: OnceLock<[Style; 5]> = OnceLock::new(); +static COLORS: OnceLock<[Style; u8::MAX as usize]> = OnceLock::new(); -pub fn get_terminal_package_colors() -> &'static [Style; 5] { +pub fn get_terminal_package_colors() -> &'static [Style; u8::MAX as usize] { COLORS.get_or_init(|| { - [ - Style::new().cyan(), - Style::new().magenta(), - Style::new().green(), - Style::new().yellow(), - Style::new().blue(), - ] + let colors: [Style; u8::MAX as usize] = + core::array::from_fn(|index| Style::new().color256(index as u8)); + + colors }) } @@ -28,7 +27,6 @@ pub struct ColorSelector { #[derive(Default)] struct ColorSelectorInner { - idx: usize, cache: HashMap, } @@ -65,13 +63,16 @@ impl ColorSelectorInner { fn insert_color(&mut self, key: String) -> &'static Style { let colors = get_terminal_package_colors(); - let chosen_color = &colors[self.idx % colors.len()]; + let color_id = (Self::get_color_id_by_key(&key) % colors.len() as u64) as usize; + let chosen_color = &colors[color_id]; // A color might have been chosen by the time we get to inserting - self.cache.entry(key).or_insert_with(|| { - // If a color hasn't been chosen, then we increment the index - self.idx += 1; - chosen_color - }) + self.cache.entry(key).or_insert_with(|| chosen_color) + } + + fn get_color_id_by_key(key: &str) -> u64 { + let mut hasher = DefaultHasher::new(); + key.hash(&mut hasher); + hasher.finish() } } @@ -110,15 +111,5 @@ mod tests { }); }); // We only inserted 2 keys so next index should be 2 - assert_eq!(selector.inner.read().unwrap().idx, 2); - } - - #[test] - fn test_color_selector_wraps_around() { - let selector = super::ColorSelector::default(); - for key in &["1", "2", "3", "4", "5", "6"] { - selector.color_for_key(key); - } - assert_eq!(selector.color_for_key("1"), selector.color_for_key("6")); } } From 67acf6ff433c6145391c1c84114c6d67b1490b0b Mon Sep 17 00:00:00 2001 From: Shahar Shalev Date: Thu, 15 Aug 2024 17:28:22 +0300 Subject: [PATCH 2/3] fix: prioritised not to take taken colors --- .../turborepo-lib/src/task_graph/visitor.rs | 4 + crates/turborepo-ui/src/color_selector.rs | 108 +++++++++++++++++- 2 files changed, 107 insertions(+), 5 deletions(-) diff --git a/crates/turborepo-lib/src/task_graph/visitor.rs b/crates/turborepo-lib/src/task_graph/visitor.rs index 2d1f5c3888742..23294ddf4c316 100644 --- a/crates/turborepo-lib/src/task_graph/visitor.rs +++ b/crates/turborepo-lib/src/task_graph/visitor.rs @@ -156,6 +156,10 @@ impl<'a> Visitor<'a> { engine: Arc, telemetry: &GenericEventBuilder, ) -> Result, Error> { + for task in engine.tasks().sorted() { + self.color_cache.color_for_key(&task.to_string()); + } + let concurrency = self.run_opts.concurrency as usize; let (node_sender, mut node_stream) = mpsc::channel(concurrency); diff --git a/crates/turborepo-ui/src/color_selector.rs b/crates/turborepo-ui/src/color_selector.rs index 3b9207c7c3269..f86e482a3e029 100644 --- a/crates/turborepo-ui/src/color_selector.rs +++ b/crates/turborepo-ui/src/color_selector.rs @@ -1,6 +1,7 @@ use std::{ collections::HashMap, hash::{DefaultHasher, Hash, Hasher}, + ops::Deref, sync::{Arc, OnceLock, RwLock}, u8, }; @@ -25,9 +26,20 @@ pub struct ColorSelector { inner: Arc>, } -#[derive(Default)] struct ColorSelectorInner { cache: HashMap, + colors_taken_state: [bool; u8::MAX as usize], + total_colors_taken: u8, +} + +impl Default for ColorSelectorInner { + fn default() -> Self { + Self { + cache: Default::default(), + colors_taken_state: [false; u8::MAX as usize], + total_colors_taken: 0, + } + } } impl ColorSelector { @@ -63,17 +75,41 @@ impl ColorSelectorInner { fn insert_color(&mut self, key: String) -> &'static Style { let colors = get_terminal_package_colors(); - let color_id = (Self::get_color_id_by_key(&key) % colors.len() as u64) as usize; - let chosen_color = &colors[color_id]; + let chosen_color = Self::get_color_id_by_key(self, &key); // A color might have been chosen by the time we get to inserting - self.cache.entry(key).or_insert_with(|| chosen_color) + self.cache + .entry(key) + .or_insert_with(|| &colors[chosen_color]) } - fn get_color_id_by_key(key: &str) -> u64 { + pub fn get_color_hash_by_key(key: &str) -> u64 { let mut hasher = DefaultHasher::new(); key.hash(&mut hasher); hasher.finish() } + + fn get_color_id_by_key(&mut self, key: &str) -> usize { + let colors = get_terminal_package_colors(); + + if self.total_colors_taken == u8::MAX { + self.colors_taken_state = [false; u8::MAX as usize]; + self.total_colors_taken = 0; + } + + let mut color_id: usize = + (Self::get_color_hash_by_key(&key) % colors.len() as u64) as usize; + + let mut state: bool = *(self.colors_taken_state.get(color_id).unwrap()); + + while state { + color_id = (color_id + 1) % colors.len(); + state = *(self.colors_taken_state.get(color_id).unwrap()); + } + + self.total_colors_taken += 1; + self.colors_taken_state[color_id] = true; + return color_id; + } } #[cfg(test)] @@ -111,5 +147,67 @@ mod tests { }); }); // We only inserted 2 keys so next index should be 2 + assert_eq!(selector.inner.read().unwrap().total_colors_taken, 2); + } + + #[test] + fn test_rotation_after_all_colors_are_taken() { + let selector = super::ColorSelector::default(); + + let colors = super::get_terminal_package_colors(); + let num_colors = colors.len(); + + // Exhaust all colors + for i in 0..num_colors { + let key = format!("package{}", i); + selector.color_for_key(&key); + } + + // At this point, all colors should be taken + for state in selector + .inner + .read() + .expect("lock poisoned") + .colors_taken_state + .iter() + .take(num_colors) + { + assert_eq!(*state, true); + } + + // The next key should start rotating from the beginning + let key_next = format!("package{}", num_colors + 1); + let next_color_id = selector.color_for_key(&key_next); + + // It should be the first color in the rotation again + let next_key_color_id = (super::ColorSelectorInner::get_color_hash_by_key(&key_next) + % colors.len() as u64) as usize; + assert_eq!(next_color_id, &colors[next_key_color_id]); + + // At this point, all colors should be not taken, expect the one taken with the + // latest package + for (index, state) in selector + .inner + .read() + .expect("lock poisoned") + .colors_taken_state + .iter() + .enumerate() + { + if index == next_key_color_id { + assert_eq!(*state, true); + } else { + assert_eq!(*state, false); + } + } + + assert_eq!( + selector + .inner + .read() + .expect("lock poisoned") + .total_colors_taken, + 1 + ); } } From dd9c265d707085b94afdff67e22421db08a40019 Mon Sep 17 00:00:00 2001 From: Shahar Shalev Date: Thu, 15 Aug 2024 17:42:41 +0300 Subject: [PATCH 3/3] fix: select colors with by task id --- crates/turborepo-lib/src/task_graph/visitor.rs | 3 ++- crates/turborepo-ui/src/color_selector.rs | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/turborepo-lib/src/task_graph/visitor.rs b/crates/turborepo-lib/src/task_graph/visitor.rs index 23294ddf4c316..6232f7e9144e6 100644 --- a/crates/turborepo-lib/src/task_graph/visitor.rs +++ b/crates/turborepo-lib/src/task_graph/visitor.rs @@ -663,6 +663,7 @@ impl<'a> ExecContextFactory<'a> { ) -> ExecContext { let task_id_for_display = self.visitor.display_task_id(&task_id); let pass_through_args = self.visitor.run_opts.args_for_task(&task_id); + let task_id_string = &task_id.to_string(); ExecContext { engine: self.engine.clone(), color_config: self.visitor.color_config, @@ -671,7 +672,7 @@ impl<'a> ExecContextFactory<'a> { pretty_prefix: self .visitor .color_cache - .prefix_with_color(&task_hash, &self.visitor.prefix(&task_id)), + .prefix_with_color(task_id_string, &self.visitor.prefix(&task_id)), task_id, task_id_for_display, task_cache, diff --git a/crates/turborepo-ui/src/color_selector.rs b/crates/turborepo-ui/src/color_selector.rs index f86e482a3e029..ee89d4e52a231 100644 --- a/crates/turborepo-ui/src/color_selector.rs +++ b/crates/turborepo-ui/src/color_selector.rs @@ -1,7 +1,6 @@ use std::{ collections::HashMap, hash::{DefaultHasher, Hash, Hasher}, - ops::Deref, sync::{Arc, OnceLock, RwLock}, u8, };