perf: Bugfixes (#38725)

Nia created

Release Notes:

- N/A

Change summary

.cargo/config.toml                       |  2 
Cargo.lock                               |  1 
crates/util_macros/src/util_macros.rs    |  8 +++-
crates/vim/Cargo.toml                    |  1 
crates/vim/src/test.rs                   | 40 ++++++++++++++++++++
crates/vim/src/test/neovim_connection.rs |  8 +++
tooling/perf/src/main.rs                 | 49 +++++++++++++++----------
7 files changed, 85 insertions(+), 24 deletions(-)

Detailed changes

.cargo/config.toml 🔗

@@ -4,7 +4,7 @@ rustflags = ["-C", "symbol-mangling-version=v0", "--cfg", "tokio_unstable"]
 
 [alias]
 xtask = "run --package xtask --"
-perf-test = ["test", "--profile", "release-fast", "--lib", "--bins", "--tests", "--config", "target.'cfg(true)'.runner='cargo run -p perf --release'", "--config", "target.'cfg(true)'.rustflags=[\"--cfg\", \"perf_enabled\"]"]
+perf-test = ["test", "--profile", "release-fast", "--lib", "--bins", "--tests", "--all-features", "--config", "target.'cfg(true)'.runner='cargo run -p perf --release'", "--config", "target.'cfg(true)'.rustflags=[\"--cfg\", \"perf_enabled\"]"]
 perf-compare = ["run", "--release", "-p", "perf", "--", "compare"]
 
 [target.x86_64-unknown-linux-gnu]

Cargo.lock 🔗

@@ -17679,6 +17679,7 @@ dependencies = [
  "multi_buffer",
  "nvim-rs",
  "parking_lot",
+ "perf",
  "picker",
  "project",
  "project_panel",

crates/util_macros/src/util_macros.rs 🔗

@@ -133,7 +133,6 @@ impl PerfArgs {
 /// Marks a test as perf-sensitive, to be triaged when checking the performance
 /// of a build. This also automatically applies `#[test]`.
 ///
-///
 /// # Usage
 /// Applying this attribute to a test marks it as average importance by default.
 /// There are 4 levels of importance (`Critical`, `Important`, `Average`, `Fluff`);
@@ -194,7 +193,12 @@ pub fn perf(our_attr: TokenStream, input: TokenStream) -> TokenStream {
         sig: mut sig_main,
         block,
     } = parse_macro_input!(input as ItemFn);
-    attrs_main.push(parse_quote!(#[test]));
+    if !attrs_main
+        .iter()
+        .any(|a| Some(&parse_quote!(test)) == a.path().segments.last())
+    {
+        attrs_main.push(parse_quote!(#[test]));
+    }
     attrs_main.push(parse_quote!(#[allow(non_snake_case)]));
 
     let fns = if cfg!(perf_enabled) {

crates/vim/Cargo.toml 🔗

@@ -66,5 +66,6 @@ parking_lot.workspace = true
 project_panel.workspace = true
 release_channel.workspace = true
 settings.workspace = true
+perf.workspace = true
 util = { workspace = true, features = ["test-support"] }
 workspace = { workspace = true, features = ["test-support"] }

crates/vim/src/test.rs 🔗

@@ -35,6 +35,7 @@ async fn test_initially_disabled(cx: &mut gpui::TestAppContext) {
     cx.assert_editor_state("hjklˇ");
 }
 
+#[perf]
 #[gpui::test]
 async fn test_neovim(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -233,6 +234,7 @@ async fn test_indent_outdent(cx: &mut gpui::TestAppContext) {
     cx.assert_editor_state("        a\nbˇ\nccc\n");
 }
 
+#[perf]
 #[gpui::test]
 async fn test_escape_command_palette(cx: &mut gpui::TestAppContext) {
     let mut cx = VimTestContext::new(cx, true).await;
@@ -348,6 +350,7 @@ async fn test_kebab_case(cx: &mut gpui::TestAppContext) {
     )
 }
 
+#[perf]
 #[gpui::test]
 async fn test_join_lines(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -434,6 +437,7 @@ async fn test_join_lines(cx: &mut gpui::TestAppContext) {
 }
 
 #[cfg(target_os = "macos")]
+#[perf]
 #[gpui::test]
 async fn test_wrapped_lines(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -585,6 +589,7 @@ async fn test_wrapped_lines(cx: &mut gpui::TestAppContext) {
     "});
 }
 
+#[perf]
 #[gpui::test]
 async fn test_folds(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -650,6 +655,7 @@ async fn test_folds(cx: &mut gpui::TestAppContext) {
     "});
 }
 
+#[perf]
 #[gpui::test]
 async fn test_folds_panic(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -685,6 +691,7 @@ async fn test_folds_panic(cx: &mut gpui::TestAppContext) {
         ˇ"});
 }
 
+#[perf]
 #[gpui::test]
 async fn test_clear_counts(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -702,6 +709,7 @@ async fn test_clear_counts(cx: &mut gpui::TestAppContext) {
         the lazy dog"});
 }
 
+#[perf]
 #[gpui::test]
 async fn test_zero(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -725,6 +733,7 @@ async fn test_zero(cx: &mut gpui::TestAppContext) {
         the lazy dog"});
 }
 
+#[perf]
 #[gpui::test]
 async fn test_selection_goal(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -741,6 +750,7 @@ async fn test_selection_goal(cx: &mut gpui::TestAppContext) {
 }
 
 #[cfg(target_os = "macos")]
+#[perf]
 #[gpui::test]
 async fn test_wrapped_motions(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -794,6 +804,7 @@ async fn test_wrapped_motions(cx: &mut gpui::TestAppContext) {
     });
 }
 
+#[perf]
 #[gpui::test]
 async fn test_wrapped_delete_end_document(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -812,6 +823,7 @@ async fn test_wrapped_delete_end_document(cx: &mut gpui::TestAppContext) {
     });
 }
 
+#[perf]
 #[gpui::test]
 async fn test_paragraphs_dont_wrap(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -859,6 +871,7 @@ async fn test_select_all_issue_2170(cx: &mut gpui::TestAppContext) {
     );
 }
 
+#[perf]
 #[gpui::test]
 async fn test_jk(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -921,6 +934,7 @@ async fn test_jk_multi(cx: &mut gpui::TestAppContext) {
     cx.assert_state("jkˇoone jkˇoone jkˇoone", Mode::Normal);
 }
 
+#[perf]
 #[gpui::test]
 async fn test_jk_delay(cx: &mut gpui::TestAppContext) {
     let mut cx = VimTestContext::new(cx, true).await;
@@ -960,6 +974,7 @@ async fn test_jk_delay(cx: &mut gpui::TestAppContext) {
     cx.assert_state("jˇkhello", Mode::Normal);
 }
 
+#[perf]
 #[gpui::test]
 async fn test_comma_w(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -1106,7 +1121,7 @@ async fn test_rename(cx: &mut gpui::TestAppContext) {
     cx.assert_state("const afterˇ = 2; console.log(after)", Mode::Normal)
 }
 
-#[perf(iterations = 1)]
+#[perf]
 #[gpui::test]
 async fn test_remap(cx: &mut gpui::TestAppContext) {
     let mut cx = VimTestContext::new(cx, true).await;
@@ -1184,6 +1199,7 @@ async fn test_remap(cx: &mut gpui::TestAppContext) {
     cx.assert_state("12ˇ 34", Mode::Normal);
 }
 
+#[perf]
 #[gpui::test]
 async fn test_undo(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -1246,6 +1262,7 @@ async fn test_mouse_selection(cx: &mut TestAppContext) {
     cx.assert_state("one «ˇtwo» three", Mode::Visual)
 }
 
+#[perf]
 #[gpui::test]
 async fn test_lowercase_marks(cx: &mut TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -1266,6 +1283,7 @@ async fn test_lowercase_marks(cx: &mut TestAppContext) {
         .assert_eq("line one\nˇtwo\nline three");
 }
 
+#[perf]
 #[gpui::test]
 async fn test_lt_gt_marks(cx: &mut TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -1342,6 +1360,7 @@ async fn test_lt_gt_marks(cx: &mut TestAppContext) {
     });
 }
 
+#[perf]
 #[gpui::test]
 async fn test_caret_mark(cx: &mut TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -1392,6 +1411,7 @@ async fn test_caret_mark(cx: &mut TestAppContext) {
 }
 
 #[cfg(target_os = "macos")]
+#[perf]
 #[gpui::test]
 async fn test_dw_eol(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -1484,6 +1504,7 @@ async fn test_toggle_comments(cx: &mut gpui::TestAppContext) {
     );
 }
 
+#[perf]
 #[gpui::test]
 async fn test_find_multibyte(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -1560,6 +1581,7 @@ async fn test_sneak(cx: &mut gpui::TestAppContext) {
     cx.assert_state(r#"11ˇ 12 13 14"#, Mode::Normal);
 }
 
+#[perf]
 #[gpui::test]
 async fn test_plus_minus(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -1579,6 +1601,7 @@ async fn test_plus_minus(cx: &mut gpui::TestAppContext) {
     cx.shared_state().await.assert_matches();
 }
 
+#[perf]
 #[gpui::test]
 async fn test_command_alias(cx: &mut gpui::TestAppContext) {
     let mut cx = VimTestContext::new(cx, true).await;
@@ -1595,6 +1618,7 @@ async fn test_command_alias(cx: &mut gpui::TestAppContext) {
     cx.set_state("ˇHello world", Mode::Normal);
 }
 
+#[perf]
 #[gpui::test]
 async fn test_remap_adjacent_dog_cat(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -1628,6 +1652,7 @@ async fn test_remap_adjacent_dog_cat(cx: &mut gpui::TestAppContext) {
     cx.shared_state().await.assert_eq("do🐱ˇ");
 }
 
+#[perf]
 #[gpui::test]
 async fn test_remap_nested_pineapple(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -1671,6 +1696,7 @@ async fn test_remap_nested_pineapple(cx: &mut gpui::TestAppContext) {
     cx.shared_state().await.assert_eq("🍍ˇ");
 }
 
+#[perf]
 #[gpui::test]
 async fn test_remap_recursion(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -1697,6 +1723,7 @@ async fn test_remap_recursion(cx: &mut gpui::TestAppContext) {
     cx.shared_state().await.assert_eq("ˇlo");
 }
 
+#[perf]
 #[gpui::test]
 async fn test_escape_while_waiting(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -1705,6 +1732,7 @@ async fn test_escape_while_waiting(cx: &mut gpui::TestAppContext) {
     cx.shared_state().await.assert_eq("ˇi");
 }
 
+#[perf]
 #[gpui::test]
 async fn test_ctrl_w_override(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -1728,6 +1756,7 @@ async fn test_visual_indent_count(cx: &mut gpui::TestAppContext) {
     cx.assert_state("    ˇhi", Mode::Normal);
 }
 
+#[perf]
 #[gpui::test]
 async fn test_record_replay_recursion(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -1740,6 +1769,7 @@ async fn test_record_replay_recursion(cx: &mut gpui::TestAppContext) {
     cx.shared_state().await.assert_eq("ˇhello world");
 }
 
+#[perf]
 #[gpui::test]
 async fn test_blackhole_register(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -1750,6 +1780,7 @@ async fn test_blackhole_register(cx: &mut gpui::TestAppContext) {
     cx.shared_state().await.assert_eq("hellˇo");
 }
 
+#[perf]
 #[gpui::test]
 async fn test_sentence_backwards(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -1825,6 +1856,7 @@ async fn test_sentence_backwards(cx: &mut gpui::TestAppContext) {
     });
 }
 
+#[perf]
 #[gpui::test]
 async fn test_sentence_forwards(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -1840,6 +1872,7 @@ async fn test_sentence_forwards(cx: &mut gpui::TestAppContext) {
     cx.set_shared_state("helˇlo.\n\n\nworld.").await;
 }
 
+#[perf]
 #[gpui::test]
 async fn test_ctrl_o_visual(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -1851,6 +1884,7 @@ async fn test_ctrl_o_visual(cx: &mut gpui::TestAppContext) {
     cx.shared_state().await.assert_eq("ˇorld.");
 }
 
+#[perf]
 #[gpui::test]
 async fn test_ctrl_o_position(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -1862,6 +1896,7 @@ async fn test_ctrl_o_position(cx: &mut gpui::TestAppContext) {
     cx.shared_state().await.assert_eq(" helloˇworld.");
 }
 
+#[perf]
 #[gpui::test]
 async fn test_ctrl_o_dot(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -2066,6 +2101,7 @@ async fn test_folded_multibuffer_excerpts(cx: &mut gpui::TestAppContext) {
     });
 }
 
+#[perf]
 #[gpui::test]
 async fn test_delete_paragraph_motion(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -2096,6 +2132,7 @@ async fn test_delete_paragraph_motion(cx: &mut gpui::TestAppContext) {
     cx.shared_clipboard().await.assert_eq("lo world.");
 }
 
+#[perf]
 #[gpui::test]
 async fn test_delete_unmatched_brace(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;
@@ -2134,6 +2171,7 @@ async fn test_delete_unmatched_brace(cx: &mut gpui::TestAppContext) {
         .assert_eq("  oth(wow)\n  oth(wow)\n");
 }
 
+#[perf]
 #[gpui::test]
 async fn test_paragraph_multi_delete(cx: &mut gpui::TestAppContext) {
     let mut cx = NeovimBackedTestContext::new(cx).await;

crates/vim/src/test/neovim_connection.rs 🔗

@@ -59,7 +59,13 @@ pub struct NeovimConnection {
 }
 
 impl NeovimConnection {
-    pub async fn new(test_case_id: String) -> Self {
+    pub async fn new(mut test_case_id: String) -> Self {
+        // When running under perf, don't create duplicate files.
+        if cfg!(perf_enabled) {
+            if test_case_id.ends_with(perf::consts::SUF_NORMAL) {
+                test_case_id.truncate(test_case_id.len() - perf::consts::SUF_NORMAL.len());
+            }
+        }
         #[cfg(feature = "neovim")]
         let handler = NvimHandler {};
         #[cfg(feature = "neovim")]

tooling/perf/src/main.rs 🔗

@@ -214,7 +214,6 @@ fn parse_mdata(t_bin: &str, mdata_fn: &str) -> Result<TestMdata, FailKind> {
 fn compare_profiles(args: &[String]) {
     let ident_new = args.first().expect("FATAL: missing identifier for new run");
     let ident_old = args.get(1).expect("FATAL: missing identifier for old run");
-    // TODO: move this to a constant also tbh
     let wspace_dir = std::env::var("CARGO_MANIFEST_DIR").unwrap();
     let runs_dir = PathBuf::from(&wspace_dir).join(consts::RUNS_DIR);
 
@@ -321,6 +320,24 @@ fn get_tests(t_bin: &str) -> impl ExactSizeIterator<Item = (String, String)> {
     out.into_iter()
 }
 
+/// Runs the specified test `count` times, returning the time taken if the test
+/// succeeded.
+#[inline]
+fn spawn_and_iterate(t_bin: &str, t_name: &str, count: NonZero<usize>) -> Option<Duration> {
+    let mut cmd = Command::new(t_bin);
+    cmd.args([t_name, "--exact"]);
+    cmd.env(consts::ITER_ENV_VAR, format!("{count}"));
+    // Don't let the child muck up our stdin/out/err.
+    cmd.stdin(Stdio::null());
+    cmd.stdout(Stdio::null());
+    cmd.stderr(Stdio::null());
+    let pre = Instant::now();
+    // Discard the output beyond ensuring success.
+    let out = cmd.spawn().unwrap().wait();
+    let post = Instant::now();
+    out.iter().find_map(|s| s.success().then_some(post - pre))
+}
+
 /// Triage a test to determine the correct number of iterations that it should run.
 /// Specifically, repeatedly runs the given test until its execution time exceeds
 /// `thresh`, calling `step(iterations)` after every failed run to determine the new
@@ -328,7 +345,8 @@ fn get_tests(t_bin: &str) -> impl ExactSizeIterator<Item = (String, String)> {
 /// else `Some(iterations)`.
 ///
 /// # Panics
-/// This will panic if `step(usize)` is not monotonically increasing.
+/// This will panic if `step(usize)` is not monotonically increasing, or if the test
+/// binary is invalid.
 fn triage_test(
     t_bin: &str,
     t_name: &str,
@@ -336,22 +354,12 @@ fn triage_test(
     mut step: impl FnMut(NonZero<usize>) -> Option<NonZero<usize>>,
 ) -> Option<NonZero<usize>> {
     let mut iter_count = DEFAULT_ITER_COUNT;
+    // It's possible that the first loop of a test might be an outlier (e.g. it's
+    // doing some caching), in which case we want to skip it.
+    let duration_once = spawn_and_iterate(t_bin, t_name, NonZero::new(1).unwrap())?;
     loop {
-        let mut cmd = Command::new(t_bin);
-        cmd.args([t_name, "--exact"]);
-        cmd.env(consts::ITER_ENV_VAR, format!("{iter_count}"));
-        // Don't let the child muck up our stdin/out/err.
-        cmd.stdin(Stdio::null());
-        cmd.stdout(Stdio::null());
-        cmd.stderr(Stdio::null());
-        let pre = Instant::now();
-        // Discard the output beyond ensuring success.
-        let out = cmd.spawn().unwrap().wait();
-        let post = Instant::now();
-        if !out.unwrap().success() {
-            break None;
-        }
-        if post - pre > thresh {
+        let duration = spawn_and_iterate(t_bin, t_name, iter_count)?;
+        if duration.saturating_sub(duration_once) > thresh {
             break Some(iter_count);
         }
         let new = step(iter_count)?;
@@ -375,7 +383,10 @@ fn hyp_profile(t_bin: &str, t_name: &str, iterations: NonZero<usize>) -> Option<
         "1",
         "--export-markdown",
         "-",
-        &format!("{t_bin} {t_name}"),
+        // Parse json instead...
+        "--time-unit",
+        "millisecond",
+        &format!("{t_bin} --exact {t_name}"),
     ]);
     perf_cmd.env(consts::ITER_ENV_VAR, format!("{iterations}"));
     let p_out = perf_cmd.output().unwrap();
@@ -390,7 +401,7 @@ fn hyp_profile(t_bin: &str, t_name: &str, iterations: NonZero<usize>) -> Option<
     // TODO: Parse json instead.
     let mut res_iter = results_line.split_whitespace();
     // Durations are given in milliseconds, so account for that.
-    let mean = Duration::from_secs_f64(res_iter.nth(4).unwrap().parse::<f64>().unwrap() / 1000.);
+    let mean = Duration::from_secs_f64(res_iter.nth(5).unwrap().parse::<f64>().unwrap() / 1000.);
     let stddev = Duration::from_secs_f64(res_iter.nth(1).unwrap().parse::<f64>().unwrap() / 1000.);
 
     Some(Timings { mean, stddev })