Skip to content

Commit 9b1fcb0

Browse files
Sewer56Sewer56
committed
Changed: Optimize absolute path resolution by canonicalizing once
For absolute paths, base.join(input) == input regardless of base, so canonicalize once and check all bases instead of per-base FS calls. Extract resolve_absolute and try_external helpers in both resolvers. Add external directory and multiple-bases benchmarks.
1 parent 3efad1f commit 9b1fcb0

4 files changed

Lines changed: 593 additions & 137 deletions

File tree

‎src/llm-coding-tools-core/benches/path_resolvers.rs‎

Lines changed: 151 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,9 @@
77
//! - `resolvers`: Compares [`AllowedPathResolver`] and [`AllowedGlobResolver`] on the same paths
88
//! - `multiple_bases`: Tests [`AllowedPathResolver`] with multiple base directories
99
//! - `canonicalize`: Isolates `canonicalize` vs `soft_canonicalize` performance
10+
//! - `external_directory`: Tests external directory permission fallback for absolute paths
1011
//!
11-
//! # Test Cases
12+
//! # Test Cases (resolvers)
1213
//!
1314
//! ```text
1415
//! | Case | Path | What it tests |
@@ -24,24 +25,48 @@
2425
//! # Reference Results (Linux, optimized build)
2526
//!
2627
//! ```text
27-
//! resolvers/AllowedPathResolver/existing_file ~1.7 µs
28-
//! resolvers/AllowedPathResolver/new_file_existing_dir ~3.5 µs (optimized: parent canonicalize)
29-
//! resolvers/AllowedPathResolver/new_file_missing_dir ~9.7 µs (fallback: soft_canonicalize)
30-
//! resolvers/AllowedPathResolver/policy_reject ~8.0 µs (no policy, resolves normally)
31-
//! resolvers/AllowedPathResolver/deep_nested ~10.9 µs
32-
//! resolvers/AllowedPathResolver/traversal_reject ~20 ns
28+
//! resolvers/AllowedPathResolver/existing_file ~2.1 µs
29+
//! resolvers/AllowedPathResolver/new_file_existing_dir ~4.1 µs (optimized: parent canonicalize)
30+
//! resolvers/AllowedPathResolver/new_file_missing_dir ~11.7 µs (fallback: soft_canonicalize)
31+
//! resolvers/AllowedPathResolver/policy_reject ~9.8 µs (no policy, resolves normally)
32+
//! resolvers/AllowedPathResolver/deep_nested ~12.7 µs
33+
//! resolvers/AllowedPathResolver/traversal_reject ~21 ns
3334
//!
34-
//! resolvers/AllowedGlobResolver_simple_policy/existing_file ~1.8 µs (overhead: ~100 ns)
35-
//! resolvers/AllowedGlobResolver_simple_policy/new_file_existing_dir ~3.5 µs (overhead: ~40 ns)
36-
//! resolvers/AllowedGlobResolver_simple_policy/new_file_missing_dir ~9.8 µs (overhead: ~90 ns)
37-
//! resolvers/AllowedGlobResolver_simple_policy/policy_reject ~54 ns (150x faster!)
38-
//! resolvers/AllowedGlobResolver_simple_policy/deep_nested ~10.8 µs
39-
//! resolvers/AllowedGlobResolver_simple_policy/traversal_reject ~28 ns
35+
//! resolvers/AllowedGlobResolver_simple_policy/existing_file ~2.3 µs (overhead: ~200 ns)
36+
//! resolvers/AllowedGlobResolver_simple_policy/new_file_existing_dir ~4.4 µs (overhead: ~300 ns)
37+
//! resolvers/AllowedGlobResolver_simple_policy/new_file_missing_dir ~12.0 µs (overhead: ~300 ns)
38+
//! resolvers/AllowedGlobResolver_simple_policy/policy_reject ~43 ns (230x faster!)
39+
//! resolvers/AllowedGlobResolver_simple_policy/deep_nested ~12.9 µs
40+
//! resolvers/AllowedGlobResolver_simple_policy/traversal_reject ~21 ns
4041
//!
41-
//! canonicalize/existing_file_canonicalize ~1.6 µs
42-
//! canonicalize/existing_file_soft_canonicalize ~4.4 µs (2.7x slower than canonicalize)
43-
//! canonicalize/new_file_shallow_soft_canonicalize ~5.9 µs
44-
//! canonicalize/new_file_deep_soft_canonicalize ~6.9 µs
42+
//! resolvers/AllowedGlobResolver_complex_policy/existing_file ~2.6 µs
43+
//! resolvers/AllowedGlobResolver_complex_policy/new_file_existing_dir ~4.6 µs
44+
//! resolvers/AllowedGlobResolver_complex_policy/new_file_missing_dir ~12.2 µs
45+
//! resolvers/AllowedGlobResolver_complex_policy/policy_reject ~105 ns
46+
//! resolvers/AllowedGlobResolver_complex_policy/deep_nested ~13.2 µs
47+
//! resolvers/AllowedGlobResolver_complex_policy/traversal_reject ~21 ns
48+
//!
49+
//! multiple_bases/first_base ~2.1 µs
50+
//! multiple_bases/second_base ~3.6 µs
51+
//! multiple_bases/third_base ~3.6 µs
52+
//! multiple_bases/not_found ~3.6 µs
53+
//!
54+
//! canonicalize/existing_file_canonicalize ~1.9 µs
55+
//! canonicalize/existing_file_soft_canonicalize ~5.3 µs (2.7x slower than canonicalize)
56+
//! canonicalize/new_file_shallow_soft_canonicalize ~7.2 µs
57+
//! canonicalize/new_file_deep_soft_canonicalize ~8.4 µs
58+
//!
59+
//! external_directory/AllowedPathResolver/external_existing_file ~548 ns (canonicalize + permission)
60+
//! external_directory/AllowedPathResolver/external_new_file ~3.3 µs (soft_canonicalize + permission)
61+
//! external_directory/AllowedPathResolver/external_rejected ~2.4 µs (canonicalize + deny)
62+
//! external_directory/AllowedPathResolver/external_no_ruleset ~2.3 µs (canonicalize, no permission)
63+
//! external_directory/AllowedPathResolver/relative_still_fails ~9.8 µs (soft_canonicalize, not external)
64+
//!
65+
//! external_directory/AllowedGlobResolver/external_existing_file ~535 ns (canonicalize + permission)
66+
//! external_directory/AllowedGlobResolver/external_new_file ~3.3 µs (soft_canonicalize + permission)
67+
//! external_directory/AllowedGlobResolver/external_rejected ~2.3 µs (canonicalize + deny)
68+
//! external_directory/AllowedGlobResolver/external_no_ruleset ~2.3 µs (canonicalize, no permission)
69+
//! external_directory/AllowedGlobResolver/relative_still_fails ~9.8 µs (soft_canonicalize, not external)
4570
//! ```
4671
//!
4772
//! # Platform Differences
@@ -68,8 +93,10 @@ use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion, Through
6893
use llm_coding_tools_core::path::{
6994
AllowedGlobResolver, AllowedPathResolver, GlobPolicy, GlobPolicyBuilder, PathResolver,
7095
};
96+
use llm_coding_tools_core::permissions::{PermissionAction, Rule, Ruleset};
7197
use soft_canonicalize::soft_canonicalize;
7298
use std::fs;
99+
use std::sync::Arc;
73100
use tempfile::TempDir;
74101

75102
const EXISTING_FILE: &str = "src/lib.rs";
@@ -279,11 +306,117 @@ fn bench_canonicalize_vs_soft(c: &mut Criterion) {
279306
group.finish();
280307
}
281308

309+
/// Benchmarks the external directory permission fallback path.
310+
///
311+
/// Tests the performance of resolving paths that are outside all base
312+
/// directories but may be allowed via an `"external_directory"` permission rule.
313+
///
314+
/// # Test Cases
315+
///
316+
/// ```text
317+
/// | Case | Description |
318+
/// |-------------------------|----------------------------------------------------------|
319+
/// | external_existing_file | Absolute path to file in allowed temp dir (fast path) |
320+
/// | external_new_file | Absolute path to new file in allowed temp dir (soft_can) |
321+
/// | external_rejected | Absolute path denied by permission ruleset (early exit) |
322+
/// | external_no_ruleset | Absolute path with no external_permission configured |
323+
/// | relative_still_fails | Relative path even when external_permission is set |
324+
/// ```
325+
fn bench_external_directory(c: &mut Criterion) {
326+
let mut group = c.benchmark_group("external_directory");
327+
328+
let current_dir = std::env::current_dir().unwrap();
329+
let external_dir = TempDir::new().unwrap();
330+
let existing_file = external_dir.path().join("existing.txt");
331+
fs::write(&existing_file, "content").unwrap();
332+
let new_file = external_dir.path().join("subdir/new_file.txt");
333+
334+
let canon_external = soft_canonicalize(external_dir.path()).unwrap();
335+
let allow_pattern = canon_external.join("*").to_str().unwrap().to_owned();
336+
337+
let mut allow_ruleset = Ruleset::new();
338+
allow_ruleset.push(
339+
Rule::new(
340+
"external_directory",
341+
allow_pattern.as_str(),
342+
PermissionAction::Allow,
343+
)
344+
.unwrap(),
345+
);
346+
347+
let mut deny_ruleset = Ruleset::new();
348+
deny_ruleset.push(Rule::new("external_directory", "*", PermissionAction::Deny).unwrap());
349+
350+
let existing_file_str = existing_file.to_str().unwrap().to_owned();
351+
let new_file_str = new_file.to_str().unwrap().to_owned();
352+
let rejected_path = std::env::temp_dir()
353+
.join("rejected_external")
354+
.join("path.txt")
355+
.to_str()
356+
.unwrap()
357+
.to_owned();
358+
359+
let allowed_path_resolver = AllowedPathResolver::new(vec![current_dir.clone()])
360+
.unwrap()
361+
.with_external_permission(Arc::new(allow_ruleset.clone()));
362+
363+
let denied_path_resolver = AllowedPathResolver::new(vec![current_dir.clone()])
364+
.unwrap()
365+
.with_external_permission(Arc::new(deny_ruleset.clone()));
366+
367+
let no_permission_resolver = AllowedPathResolver::new(vec![current_dir.clone()]).unwrap();
368+
369+
let allowed_glob_resolver = AllowedGlobResolver::new(vec![current_dir.clone()])
370+
.unwrap()
371+
.with_external_permission(Arc::new(allow_ruleset));
372+
373+
let denied_glob_resolver = AllowedGlobResolver::new(vec![current_dir.clone()])
374+
.unwrap()
375+
.with_external_permission(Arc::new(deny_ruleset));
376+
377+
let no_permission_glob_resolver = AllowedGlobResolver::new(vec![current_dir.clone()]).unwrap();
378+
379+
group.throughput(Throughput::Elements(1));
380+
381+
for (case_name, path_input) in [
382+
("external_existing_file", existing_file_str.as_str()),
383+
("external_new_file", new_file_str.as_str()),
384+
("external_rejected", rejected_path.as_str()),
385+
("external_no_ruleset", rejected_path.as_str()),
386+
("relative_still_fails", "relative/path.txt"),
387+
] {
388+
let (path_resolver, glob_resolver) = match case_name {
389+
"external_existing_file" | "external_new_file" => {
390+
(&allowed_path_resolver, &allowed_glob_resolver)
391+
}
392+
"external_rejected" => (&denied_path_resolver, &denied_glob_resolver),
393+
"external_no_ruleset" => (&no_permission_resolver, &no_permission_glob_resolver),
394+
"relative_still_fails" => (&allowed_path_resolver, &allowed_glob_resolver),
395+
_ => unreachable!(),
396+
};
397+
398+
group.bench_with_input(
399+
BenchmarkId::new("AllowedPathResolver", case_name),
400+
path_resolver,
401+
|b, resolver| b.iter(|| resolver.resolve(black_box(path_input))),
402+
);
403+
404+
group.bench_with_input(
405+
BenchmarkId::new("AllowedGlobResolver", case_name),
406+
glob_resolver,
407+
|b, resolver| b.iter(|| resolver.resolve(black_box(path_input))),
408+
);
409+
}
410+
411+
group.finish();
412+
}
413+
282414
criterion_group!(
283415
benches,
284416
bench_resolvers_same_paths,
285417
bench_multiple_bases,
286-
bench_canonicalize_vs_soft
418+
bench_canonicalize_vs_soft,
419+
bench_external_directory
287420
);
288421

289422
criterion_main!(benches);

0 commit comments

Comments
 (0)