From e19ad08c6dff5116ae99dd1bed43f151757640f8 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 20 Mar 2026 20:39:28 +1100 Subject: [PATCH] Use enums to clarify `DepNodeColorMap` color marking When a function's documentation has to explain the meaning of nested results and options, then it is often a good candidate for using a custom result enum instead. This commit also renames `DepNodeColorMap::try_mark` to `try_set_color`, to make it more distinct from the similarly-named `DepGraph::try_mark_green`. The difference is that `try_mark_green` is a higher-level operation that tries to determine whether a node _can_ be marked green, whereas `try_set_color` is a lower-level operation that actually records a color for the node. --- compiler/rustc_middle/src/dep_graph/graph.rs | 51 ++++++++++++++----- .../rustc_middle/src/dep_graph/serialized.rs | 25 +++++---- typos.toml | 1 + 3 files changed, 52 insertions(+), 25 deletions(-) diff --git a/compiler/rustc_middle/src/dep_graph/graph.rs b/compiler/rustc_middle/src/dep_graph/graph.rs index d0d7d581b439..7823e9d62f1f 100644 --- a/compiler/rustc_middle/src/dep_graph/graph.rs +++ b/compiler/rustc_middle/src/dep_graph/graph.rs @@ -1415,28 +1415,29 @@ pub(super) fn current(&self, index: SerializedDepNodeIndex) -> Option Result<(), Option> { - let value = &self.values[prev_index]; - match value.compare_exchange( + color: DesiredColor, + ) -> TrySetColorResult { + match self.values[prev_index].compare_exchange( COMPRESSED_UNKNOWN, - if green { index.as_u32() } else { COMPRESSED_RED }, + match color { + DesiredColor::Red => COMPRESSED_RED, + DesiredColor::Green { index } => index.as_u32(), + }, Ordering::Relaxed, Ordering::Relaxed, ) { - Ok(_) => Ok(()), - Err(v) => Err(if v == COMPRESSED_RED { None } else { Some(DepNodeIndex::from_u32(v)) }), + Ok(_) => TrySetColorResult::Success, + Err(COMPRESSED_RED) => TrySetColorResult::AlreadyRed, + Err(index) => TrySetColorResult::AlreadyGreen { index: DepNodeIndex::from_u32(index) }, } } @@ -1463,6 +1464,28 @@ pub(super) fn insert_red(&self, index: SerializedDepNodeIndex) { } } +/// The color that [`DepNodeColorMap::try_set_color`] should try to apply to a node. +#[derive(Clone, Copy, Debug)] +pub(super) enum DesiredColor { + /// Try to mark the node red. + Red, + /// Try to mark the node green, associating it with a current-session node index. + Green { index: DepNodeIndex }, +} + +/// Return value of [`DepNodeColorMap::try_set_color`], indicating success or failure, +/// and (on failure) what the existing color is. +#[derive(Clone, Copy, Debug)] +pub(super) enum TrySetColorResult { + /// The [`DesiredColor`] was freshly applied to the node. + Success, + /// Coloring failed because the node was already marked red. + AlreadyRed, + /// Coloring failed because the node was already marked green, + /// and corresponds to node `index` in the current-session dep graph. + AlreadyGreen { index: DepNodeIndex }, +} + #[inline(never)] #[cold] pub(crate) fn print_markframe_trace(graph: &DepGraph, frame: &MarkFrame<'_>) { diff --git a/compiler/rustc_middle/src/dep_graph/serialized.rs b/compiler/rustc_middle/src/dep_graph/serialized.rs index 8a4ac4b5e5ac..7a8d25d36744 100644 --- a/compiler/rustc_middle/src/dep_graph/serialized.rs +++ b/compiler/rustc_middle/src/dep_graph/serialized.rs @@ -58,7 +58,7 @@ use rustc_session::Session; use tracing::{debug, instrument}; -use super::graph::{CurrentDepGraph, DepNodeColorMap}; +use super::graph::{CurrentDepGraph, DepNodeColorMap, DesiredColor, TrySetColorResult}; use super::retained::RetainedDepGraph; use super::{DepKind, DepNode, DepNodeIndex}; use crate::dep_graph::edges::EdgesVec; @@ -905,13 +905,14 @@ pub(crate) fn send_and_color( let mut local = self.status.local.borrow_mut(); let index = self.status.next_index(&mut *local); + let color = if is_green { DesiredColor::Green { index } } else { DesiredColor::Red }; - // Use `try_mark` to avoid racing when `send_promoted` is called concurrently + // Use `try_set_color` to avoid racing when `send_promoted` is called concurrently // on the same index. - match colors.try_mark(prev_index, index, is_green) { - Ok(()) => (), - Err(None) => panic!("dep node {:?} is unexpectedly red", prev_index), - Err(Some(dep_node_index)) => return dep_node_index, + match colors.try_set_color(prev_index, color) { + TrySetColorResult::Success => {} + TrySetColorResult::AlreadyRed => panic!("dep node {prev_index:?} is unexpectedly red"), + TrySetColorResult::AlreadyGreen { index } => return index, } self.status.bump_index(&mut *local); @@ -923,7 +924,8 @@ pub(crate) fn send_and_color( /// from the previous dep graph and expects all edges to already have a new dep node index /// assigned. /// - /// This will also ensure the dep node is marked green if `Some` is returned. + /// Tries to mark the dep node green, and returns Some if it is now green, + /// or None if had already been concurrently marked red. #[inline] pub(crate) fn send_promoted( &self, @@ -935,10 +937,10 @@ pub(crate) fn send_promoted( let mut local = self.status.local.borrow_mut(); let index = self.status.next_index(&mut *local); - // Use `try_mark_green` to avoid racing when `send_promoted` or `send_and_color` + // Use `try_set_color` to avoid racing when `send_promoted` or `send_and_color` // is called concurrently on the same index. - match colors.try_mark(prev_index, index, true) { - Ok(()) => { + match colors.try_set_color(prev_index, DesiredColor::Green { index }) { + TrySetColorResult::Success => { self.status.bump_index(&mut *local); self.status.encode_promoted_node( index, @@ -949,7 +951,8 @@ pub(crate) fn send_promoted( ); Some(index) } - Err(dep_node_index) => dep_node_index, + TrySetColorResult::AlreadyRed => None, + TrySetColorResult::AlreadyGreen { index } => Some(index), } } diff --git a/typos.toml b/typos.toml index 82e2b98f2e49..8e14ce58dcb4 100644 --- a/typos.toml +++ b/typos.toml @@ -48,6 +48,7 @@ unstalled = "unstalled" # short for un-stalled # the non-empty form can be automatically fixed by `--bless`. # # tidy-alphabetical-start +atomicaly = "atomically" definitinon = "definition" dependy = "" similarlty = "similarity"