Skip to content

Commit 4ed5aa3

Browse files
lpetreclaude
andauthored
Coalesce per-file plugins into one salsa query per file (#308)
`per_file_plugin_ops` was keyed `(file, plugin_id)`, so N registered per-file plugins meant N salsa queries (and N memo entries) per file — the salsa bookkeeping (memo table, dependency edges, per-build revalidation) grew O(files × plugins). Re-key it on `(file, set_id)`, where `set_id` interns the ordered, deduped per-file plugin id list, and run the whole set inside the one query body. Salsa work is now O(files); the per-plugin loop is plain Rust, free of salsa. * New `PER_FILE_PLUGIN_SETS` registry interns an ordered id list to a stable `set_id` (FxHasher over the list). Interning by the list keeps it sound across `Analysis`es with different plugin sets — a different list → a different `set_id` → separate memo entries — and identical sets (incl. across `re_materialize`) share entries. Same append-only, immutable-per-id contract as the configured/external registries, so the untracked lookup from inside the query is sound. * `extract_per_file_plugin_set` dedups by id preserving registration order, exactly reproducing the dedup the old `(file, id)` key gave for free (two `main_block()`s, or identical `ServerConfig` configs that intern to one id, run once). * Populate warms one query per file; assemble reads one slice per file. The shared dispatch is factored into `run_one_per_file`. No support for the plugin list changing mid-session is added (per the design): invalidation stays purely per-file via the file's tracked inputs. Byte-identical output (A/B on the framework corpus with all 17 builtin plugins), full suite (745) green incl. the ServerConfig cache-interning tests. https://claude.ai/code/session_01KYNAs8Y9ctXsM1waLmXEaP Co-authored-by: Claude <noreply@anthropic.com>
1 parent ca1c38f commit 4ed5aa3

2 files changed

Lines changed: 129 additions & 58 deletions

File tree

runtime/src/native_plugins.rs

Lines changed: 81 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -943,41 +943,105 @@ fn configured_per_file_plugin(id: u32) -> Option<Arc<ConfiguredPerFile>> {
943943
.map(Arc::clone)
944944
}
945945

946-
/// Salsa-tracked per-file plugin invocation. Keyed on ``(file, id)``;
947-
/// re-runs only when the file's tracked inputs (``file_to_nodes`` /
948-
/// ``parsed_module`` / ``line_index``) change. Returns the file-local ops
949-
/// the harness translates to global indices at apply time.
950-
#[salsa::tracked(returns(ref), heap_size = ruff_memory_usage::heap_size)]
951-
pub(crate) fn per_file_plugin_ops(
952-
db: &dyn ProjectDb,
953-
file: File,
946+
/// Process-global registry interning an *ordered* per-file plugin id list to a
947+
/// stable `set_id`. This is what lets [`per_file_plugin_ops`] be keyed
948+
/// `(file, set_id)` — **one** salsa query per file that runs the whole set —
949+
/// rather than `(file, plugin)`, one per plugin. Salsa's per-build memo work
950+
/// is then O(files), not O(files × plugins): the per-plugin loop lives inside
951+
/// the query body, costing nothing in salsa. Interning by the ordered list
952+
/// keeps it sound across `Analysis`es with different plugin sets (a different
953+
/// list → a different `set_id` → separate memo entries), and an identical set
954+
/// reconstructed across a `re_materialize` reuses its `set_id` (and so its
955+
/// cache entries). Append-only and immutable for a given `set_id`, so the
956+
/// untracked lookup from inside the query is sound — same contract as
957+
/// [`CONFIGURED_PER_FILE_PLUGINS`] / [`EXTERNAL_PER_FILE_PLUGINS`].
958+
static PER_FILE_PLUGIN_SETS: std::sync::OnceLock<std::sync::RwLock<PluginSetRegistry>> =
959+
std::sync::OnceLock::new();
960+
961+
#[derive(Default)]
962+
struct PluginSetRegistry {
963+
sets: Vec<Arc<[PerFilePluginId]>>,
964+
by_hash: FxHashMap<u64, u32>,
965+
}
966+
967+
/// Register (intern) an ordered per-file plugin id list, returning its
968+
/// process-stable `set_id`. An identical list returns the existing id.
969+
pub(crate) fn register_per_file_set(ids: Vec<PerFilePluginId>) -> u32 {
970+
let reg =
971+
PER_FILE_PLUGIN_SETS.get_or_init(|| std::sync::RwLock::new(PluginSetRegistry::default()));
972+
let mut hasher = FxHasher::default();
973+
ids.hash(&mut hasher);
974+
let hash = hasher.finish();
975+
let mut guard = reg.write().expect("per-file plugin set registry poisoned");
976+
if let Some(&id) = guard.by_hash.get(&hash) {
977+
return id;
978+
}
979+
let set_id = guard.sets.len() as u32;
980+
guard.sets.push(ids.into());
981+
guard.by_hash.insert(hash, set_id);
982+
set_id
983+
}
984+
985+
/// Look up an interned per-file plugin id list by `set_id`.
986+
fn per_file_set(set_id: u32) -> Option<Arc<[PerFilePluginId]>> {
987+
PER_FILE_PLUGIN_SETS
988+
.get()?
989+
.read()
990+
.expect("per-file plugin set registry poisoned")
991+
.sets
992+
.get(set_id as usize)
993+
.map(Arc::clone)
994+
}
995+
996+
/// Run one per-file plugin (configless builtin, configured builtin, or
997+
/// external dylib) into `ops` — the single curated [`plugin_api::PerFilePlugin`]
998+
/// dispatch, shared by the whole-set loop in [`per_file_plugin_ops`].
999+
fn run_one_per_file(
9541000
id: PerFilePluginId,
955-
) -> Vec<FileLocalOp> {
956-
let file_ctx = FileContext::new(db, file);
957-
let pctx = plugin_api::PluginFileCtx::new(&file_ctx);
958-
let mut ops = plugin_api::FileOps::new();
959-
// Every per-file plugin — configless builtin, configured builtin, or
960-
// external dylib — runs through the one curated `PerFilePlugin` surface.
1001+
pctx: &plugin_api::PluginFileCtx<'_>,
1002+
ops: &mut plugin_api::FileOps,
1003+
) {
9611004
match id {
962-
PerFilePluginId::Builtin(kind) => kind.plugin().run_on_file(&pctx, &mut ops),
1005+
PerFilePluginId::Builtin(kind) => kind.plugin().run_on_file(pctx, ops),
9631006
PerFilePluginId::Configured(plugin_id) => {
9641007
// Resolve the configured plugin from the registry; a stale id
9651008
// (shouldn't happen) yields no ops.
9661009
if let Some(cfg) = configured_per_file_plugin(plugin_id) {
9671010
let per_file: &dyn plugin_api::PerFilePlugin = cfg.as_ref();
968-
per_file.run_on_file(&pctx, &mut ops);
1011+
per_file.run_on_file(pctx, ops);
9691012
}
9701013
}
9711014
PerFilePluginId::External(plugin_id) => {
9721015
// Resolve the plugin and its per-file capability from the
9731016
// registry; a stale id (shouldn't happen) yields no ops.
9741017
if let Some(plugin) = external_per_file_plugin(plugin_id) {
9751018
if let Some(per_file) = plugin.per_file() {
976-
per_file.run_on_file(&pctx, &mut ops);
1019+
per_file.run_on_file(pctx, ops);
9771020
}
9781021
}
9791022
}
9801023
}
1024+
}
1025+
1026+
/// Salsa-tracked per-file plugin invocation. Keyed on ``(file, set_id)`` — the
1027+
/// whole registered per-file plugin *set* runs in one query, so re-runs are
1028+
/// O(files) not O(files × plugins). Re-runs only when the file's tracked
1029+
/// inputs (``file_to_nodes`` / ``parsed_module`` / ``line_index``) change.
1030+
/// Returns every plugin's file-local ops concatenated in registration order
1031+
/// (the order the harness translates + folds at apply time).
1032+
#[salsa::tracked(returns(ref), heap_size = ruff_memory_usage::heap_size)]
1033+
pub(crate) fn per_file_plugin_ops(db: &dyn ProjectDb, file: File, set_id: u32) -> Vec<FileLocalOp> {
1034+
let file_ctx = FileContext::new(db, file);
1035+
let pctx = plugin_api::PluginFileCtx::new(&file_ctx);
1036+
let mut ops = plugin_api::FileOps::new();
1037+
// Every per-file plugin in the set — configless builtin, configured
1038+
// builtin, or external dylib — runs through the one curated
1039+
// `PerFilePlugin` surface, in registration order.
1040+
if let Some(ids) = per_file_set(set_id) {
1041+
for &id in ids.iter() {
1042+
run_one_per_file(id, &pctx, &mut ops);
1043+
}
1044+
}
9811045
ops.into_inner()
9821046
}
9831047

runtime/src/project.rs

Lines changed: 48 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ impl Project {
9797
false,
9898
None,
9999
&counters,
100-
&[],
100+
None,
101101
GraphBuilder::with_capacity(0),
102102
ResolveCache::default(),
103103
None,
@@ -376,7 +376,7 @@ pub(crate) fn build_project_graph(
376376
show_progress: bool,
377377
stack_size: Option<usize>,
378378
counters: &Arc<ProgressCounters>,
379-
per_file_plugin_ids: &[crate::native_plugins::PerFilePluginId],
379+
per_file_set_id: Option<u32>,
380380
carry: GraphBuilder,
381381
cache: ResolveCache,
382382
scope: Option<Vec<File>>,
@@ -552,7 +552,6 @@ pub(crate) fn build_project_graph(
552552
// commit message for the hazard.
553553
let dist_db: ProjectDatabase = db.clone();
554554
let files_ref: &[File] = &populate_files;
555-
let per_file_ids_ref: &[crate::native_plugins::PerFilePluginId] = per_file_plugin_ids;
556555
let counters_ref = Arc::clone(counters);
557556
let run_populate = move || {
558557
use salsa::Database as _;
@@ -599,14 +598,17 @@ pub(crate) fn build_project_graph(
599598
// reads.
600599
let _ = file_extraction(local_db, file);
601600
// Per-file native plugins: warm the salsa-cached
602-
// `per_file_plugin_ops(file, id)` query on this
603-
// worker so the serial assembly fold below is a
604-
// pure cache read. `run_on_file` is GIL-free and
605-
// touches only this file, so it composes with the
606-
// GIL-released fan-out.
607-
for &id in per_file_ids_ref {
601+
// `per_file_plugin_ops(file, set_id)` query on
602+
// this worker so the assembly fold below is a
603+
// pure cache read. One query runs the whole
604+
// plugin set (keyed on `set_id`), so this is O(1)
605+
// in plugin count, not one warm per plugin.
606+
// `run_on_file` is GIL-free and touches only this
607+
// file, so it composes with the GIL-released
608+
// fan-out.
609+
if let Some(set_id) = per_file_set_id {
608610
let _ = crate::native_plugins::per_file_plugin_ops(
609-
local_db, file, id,
611+
local_db, file, set_id,
610612
);
611613
}
612614
// Every per-file AST consumer for this file has
@@ -688,7 +690,7 @@ pub(crate) fn build_project_graph(
688690
db,
689691
&project_files,
690692
&peer_pyi_to_py,
691-
per_file_plugin_ids,
693+
per_file_set_id,
692694
counters,
693695
&reload_log,
694696
carry,
@@ -879,7 +881,7 @@ fn assemble_graph<'db>(
879881
db: &'db ProjectDatabase,
880882
project_files: &[File],
881883
peer_pyi_to_py: &FxHashMap<File, File>,
882-
per_file_plugin_ids: &[crate::native_plugins::PerFilePluginId],
884+
per_file_set_id: Option<u32>,
883885
counters: &Arc<ProgressCounters>,
884886
reload_log: &ReloadLog,
885887
carry: GraphBuilder,
@@ -1049,23 +1051,16 @@ fn assemble_graph<'db>(
10491051
for &file in project_files {
10501052
spec_payloads.push(file_to_refspecs(db, file));
10511053
}
1052-
// Per-file plugin ops, flattened across the registered per-file
1053-
// plugins (in registration order). Prefetched here so the per-part
1054-
// edge translation below can run db-free on rayon workers.
1055-
let plugin_ops: Vec<Vec<&'db crate::native_plugins::FileLocalOp>> = if per_file_plugin_ids
1056-
.is_empty()
1057-
{
1058-
vec![Vec::new(); n_files]
1059-
} else {
1060-
project_files
1054+
// Per-file plugin ops: one salsa query per file runs the whole plugin
1055+
// set (keyed on `set_id`) and returns every plugin's ops concatenated in
1056+
// registration order. Prefetched here (a cache read — warmed in populate)
1057+
// so the per-part edge translation below can run db-free on rayon workers.
1058+
let plugin_ops: Vec<&'db [crate::native_plugins::FileLocalOp]> = match per_file_set_id {
1059+
Some(set_id) => project_files
10611060
.iter()
1062-
.map(|&file| {
1063-
per_file_plugin_ids
1064-
.iter()
1065-
.flat_map(|&id| crate::native_plugins::per_file_plugin_ops(db, file, id).iter())
1066-
.collect()
1067-
})
1068-
.collect()
1061+
.map(|&file| crate::native_plugins::per_file_plugin_ops(db, file, set_id).as_slice())
1062+
.collect(),
1063+
None => vec![&[][..]; n_files],
10691064
};
10701065

10711066
// Per-file anchor-directory ids. ty's `resolve_module` is anchor-
@@ -2112,15 +2107,15 @@ fn assemble_graph<'db>(
21122107
// derived state, rebuilt from every file's ops each pass.
21132108
let mut topic_facts: FxHashMap<String, Vec<crate::native_plugins::plugin_api::Fact>> =
21142109
FxHashMap::default();
2115-
if !per_file_plugin_ids.is_empty() {
2110+
if per_file_set_id.is_some() {
21162111
use crate::native_plugins::FileLocalOp;
21172112
for (pos, ops) in plugin_ops.iter().enumerate() {
21182113
let mint = &mints[pos];
21192114
let len = mint.payload.nodes.len();
21202115
let to_global =
21212116
|local: u32| ((local as usize) < len).then_some(mint.base + local as usize);
21222117
let mut file_path: Option<String> = None;
2123-
for op in ops {
2118+
for op in ops.iter() {
21242119
match op {
21252120
FileLocalOp::Edge { .. } => {}
21262121
FileLocalOp::Entrypoint { decl_local_idx } => {
@@ -2275,7 +2270,7 @@ fn assemble_graph<'db>(
22752270
{
22762271
use crate::native_plugins::FileLocalOp;
22772272
let len = mint.payload.nodes.len();
2278-
for op in &plugin_ops_ref[pos] {
2273+
for op in plugin_ops_ref[pos] {
22792274
if let FileLocalOp::Edge {
22802275
src_local_idx,
22812276
dst_local_idx,
@@ -2573,26 +2568,38 @@ fn run_job<T: Sync, R: Send>(
25732568
/// post-build plugin pass. Project-wide plugins (including project-wide
25742569
/// external dylibs) and non-native Python plugins are skipped here —
25752570
/// they still run in [`collect_prepared_plugin_ops`].
2576-
fn extract_per_file_plugin_ids(
2577-
py: Python<'_>,
2578-
plugins: &[PyObject],
2579-
) -> Vec<crate::native_plugins::PerFilePluginId> {
2571+
/// Collect the registered per-file plugins (in registration order) and intern
2572+
/// the ordered id list to a single process-stable `set_id`. `None` when no
2573+
/// per-file plugins are registered (the build skips the per-file pass). The
2574+
/// whole set then rides one salsa query per file keyed on this `set_id` — see
2575+
/// [`crate::native_plugins::per_file_plugin_ops`].
2576+
fn extract_per_file_plugin_set(py: Python<'_>, plugins: &[PyObject]) -> Option<u32> {
25802577
use crate::native_plugins::{NativePlugin, NativePluginKind, PerFilePluginId};
2581-
let mut ids = Vec::new();
2578+
let mut ids: Vec<PerFilePluginId> = Vec::new();
2579+
let mut seen: FxHashSet<PerFilePluginId> = FxHashSet::default();
2580+
// Dedup by id, preserving first-occurrence (registration) order. Two
2581+
// plugins with the same id — e.g. identical `ServerConfig` configs that
2582+
// intern to one `Configured` id, or two `main_block()`s — collapse to one
2583+
// run, exactly as the old `(file, id)` salsa key deduped them.
2584+
let mut push = |id: PerFilePluginId, ids: &mut Vec<PerFilePluginId>| {
2585+
if seen.insert(id) {
2586+
ids.push(id);
2587+
}
2588+
};
25822589
for p in plugins {
25832590
let Ok(native) = p.bind(py).downcast::<NativePlugin>() else {
25842591
continue;
25852592
};
25862593
match &native.borrow().kind {
2587-
NativePluginKind::PerFile(id) => ids.push(*id),
2594+
NativePluginKind::PerFile(id) => push(*id, &mut ids),
25882595
NativePluginKind::External {
25892596
per_file_id: Some(eid),
25902597
..
2591-
} => ids.push(PerFilePluginId::External(*eid)),
2598+
} => push(PerFilePluginId::External(*eid), &mut ids),
25922599
_ => {}
25932600
}
25942601
}
2595-
ids
2602+
(!ids.is_empty()).then(|| crate::native_plugins::register_per_file_set(ids))
25962603
}
25972604

25982605
/// Owned `(module, name, anchor-dir)` → resolved class-base memo.
@@ -3398,7 +3405,7 @@ impl ProjectContext {
33983405
let counters = Arc::clone(&slf.borrow(py).progress);
33993406
let build_result = {
34003407
let mut this = slf.borrow_mut(py);
3401-
let per_file_ids = extract_per_file_plugin_ids(py, &this.plugins);
3408+
let per_file_set_id = extract_per_file_plugin_set(py, &this.plugins);
34023409
// Hand the previous build's resolve cache + the
34033410
// accumulated change scope to the new build. The cache
34043411
// moves out (the old outputs keep an invalid Default);
@@ -3422,7 +3429,7 @@ impl ProjectContext {
34223429
show_progress,
34233430
stack_size,
34243431
&counters,
3425-
&per_file_ids,
3432+
per_file_set_id,
34263433
carry,
34273434
cache,
34283435
scope,

0 commit comments

Comments
 (0)