Speed up tidy again

[the8472]

Tidy has slowed down quite a bit over the years (#81833, #105829), so let's recover some perf

OLD

$ hyperfine -w 1 "./x test tidy"
Benchmark #1: ./x test tidy
  Time (mean ± σ):      5.076 s ±  0.057 s    [User: 12.724 s, System: 6.707 s]
  Range (min … max):    4.972 s …  5.148 s    10 runs

$ taskset -c 0-5 hyperfine -w 1 "./x test tidy"
Benchmark #1: ./x test tidy
  Time (mean ± σ):      7.973 s ±  0.099 s    [User: 12.072 s, System: 5.550 s]
  Range (min … max):    7.838 s …  8.184 s    10 runs

NEW

$ hyperfine -w 1 "./x test tidy"
Benchmark #1: ./x test tidy
  Time (mean ± σ):      3.134 s ±  0.055 s    [User: 7.993 s, System: 6.842 s]
  Range (min … max):    3.047 s …  3.204 s    10 runs

$ taskset -c 0-5 hyperfine -w 1 "./x test tidy"
Benchmark #1: ./x test tidy
  Time (mean ± σ):      3.921 s ±  0.129 s    [User: 7.204 s, System: 5.563 s]
  Range (min … max):    3.740 s …  4.069 s    10 runs

After this about half of the remaining time is burned by bootstrap doing git and cargo stuff.

Fixes: #141074

---

LLM disclosure: I used copilot's autocomplete. Most suggestions were trivial (or discarded for being incorrect), with the exception of the Semaphore type which it produced almost wholesale. I compared it to implementations in the wild and they look almost identical, so I hope this too falls under "trivial".

[rustbot]

tidy extra checks were modified.

cc @lolbinarycat

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @BoxyUwU

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: bootstrap
• bootstrap expanded to 6 candidates
• Random selection from Mark-Simulacrum, clubby789, jieyouxu

[Kobzol]

Thank you! Left some comments.

I don't love implementing our own synchronization primitives in tidy, even if they are supposedly simple 😆 I wonder, what do you think about piggybacking upon higher-level sync primitives (a channel) instead?

pub struct Semaphore {
    tx: std::sync::mpsc::SyncSender<()>,
    rx: std::sync::Mutex<std::sync::mpsc::Receiver<()>>,
}

impl Semaphore {
    pub fn new(count: NonZeroUsize) -> Self {
        let (tx, rx) = std::sync::mpsc::sync_channel(count.into());
        for _ in 0..count.into() {
            tx.send(()).unwrap();
        }
        Self { tx, rx: std::sync::Mutex::new(rx) }
    }

    pub fn acquire(&self) -> Guard<'_> {
        let _token = self.rx.lock().unwrap().recv().expect("Cannot read token");
        Guard { _sem: self, tx: self.tx.clone() }
    }
}

pub struct Guard<'a> {
    _sem: &'a Semaphore,
    tx: std::sync::mpsc::SyncSender<()>,
}

impl Drop for Guard<'_> {
    fn drop(&mut self) {
        // Return the token back
        self.tx.send(()).unwrap();
    }
}

View changes since this review

[Kobzol]

Is this really measurable? Debug asserts could in theory help find some overflows or similar things. I don't think that we should trade this off.

[the8472]

The asserts showed up in profiles but I haven't measured the change in isolation, will do.

The thing is that we run bootstrap in dev as if it were our release profile... would adding a test profile which reenables them be sufficient?

[Kobzol]

Well we only do that because of compile times, nothing else, really. I guess that we could only use assertions on CI, but I'd like to see if the assertions really make that much of a difference.

[Kobzol]

There is already the same function called start_capture. I think that you don't want to capture here, though? 🤔

[Kobzol]

Why does this create a new scope when one scope is already passed in?

[the8472]

Some leftover from trying different approaches. I'll remove either the local scope or the passed argument.

[Kobzol]

Why is the semaphore not acquired for the first workspace?

[the8472]

If concurrency==1 then that would hang because the deps module itself already takes up one task slot.

[Kobzol]

I see, then please add this as a comment on top of this line.

[Kobzol]

Btw, when I run tidy several times in a row, with the disk cache primed, on main the duration is ~9s, while with this PR it is ~8s.

[Kobzol]

@rustbot author