Try to make ObligationForest more efficient

[bugadani]

This PR tries to decrease the number of allocations in ObligationForest, as well as moves some cold path code to an uninlined function.

[rust-highfive]

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

[petrochenkov]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit b23fdd325dfe20c9ccc0dff4197c40cf6a2952eb with merge b26b61d0fd2de6070a1e622018568765eae31abb...

[petrochenkov]

Didn't notice the failing CI, the try build is going to fail.
Needs green CI to run perf.

[bugadani]

The CI fails because the tests don't compile, I don't know if perf requires that. Working on it :)

[bugadani]

@petrochenkov should be good now.

[Aaron1011]

@bors try

[bors]

⌛ Trying commit 853756a9d69bbed097b29a3b1b7147883750c624 with merge 6ac291c1bb097b0dfcfc216f6387b7e4cb80c38e...

[bors]

☀️ Try build successful - checks-actions, checks-azure
Build commit: 6ac291c1bb097b0dfcfc216f6387b7e4cb80c38e (6ac291c1bb097b0dfcfc216f6387b7e4cb80c38e)

[rust-timer]

Queued 6ac291c1bb097b0dfcfc216f6387b7e4cb80c38e with parent adef9da, future comparison URL.

[rust-timer]

Finished benchmarking try commit (6ac291c1bb097b0dfcfc216f6387b7e4cb80c38e): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

[bugadani]

Can I request another perf run, please?

I'm not entirely sure about the previous run, but I don't have any better guess than the change where I replaced a Vec of errors to a closure. I have reverted this change.
In the last commit, I'm trying to replace a runtime condition with compiler inlined no-ops.

[petrochenkov]

@bors try @rust-timer queue

FYI, a lot of effort went into benchmarking and performance-tuning this code in the past, see the git history for obligation forest, commits by nnethercote in particular.
So it's quite possible that something like this PR was already tried and rejected.

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 0bf5cbd06c3c8cc42d555f37c81e59358097bb17 with merge e6a746c02317eef2240ba530ecedd948da7d7d50...

[bugadani]

@petrochenkov thanks! I'm not trying to claim that I can do better in a day, I don't plan on pushing this further if the results aren't good enough. But seeing the results can be an educational experience for sure.

[bors]

☀️ Try build successful - checks-actions, checks-azure
Build commit: e6a746c02317eef2240ba530ecedd948da7d7d50 (e6a746c02317eef2240ba530ecedd948da7d7d50)

[rust-timer]

Queued e6a746c02317eef2240ba530ecedd948da7d7d50 with parent 5565241, future comparison URL.

[panstromek]

Just in case you didn't notice already, there's also another big PR open to improve perf for Obligation Forest. #69218

[rust-timer]

Finished benchmarking try commit (e6a746c02317eef2240ba530ecedd948da7d7d50): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

[bugadani]

That looks better 😅

[petrochenkov]

I'm not familiar with obligation forest, but people who are familiar are mostly unavailable, so I'll try to review this during the weekend, the changes don't look too invasive.
cc @nnethercote just in case

[bugadani]

@petrochenkov Thanks!

With the last force push I've removed a commit-revert pair that doesn't need to be there.

[nnethercote]

These look ok to me. This will interfere with #69218, but that PR is in a very uncertain state so I think it's reasonable to go forward with these concrete improvements.

@bugadani: How did you find these improvements? Did you use a profiler, and if so, which one? I'm alway curious to learn how other people find optimizations :)

[bugadani]

@nnethercote in this case this was complete guesswork. I was looking a piece of related code somewhere, wanted to understand what it does, and stumbled upon the comments in OF mentioning the code is hot. From there, I went on looking if the implementation does work it doesn't necessarily have to. In this case, this boiled down to "no allocation is the fastest type of allocation".

Of course, this approach has two downsides:

• results can go both ways
• results are usually minor, on the borderline insignificant levels for a software of this scope.

There still are some unnecessary iterator -> vector -> iterator flows around process_obligation, which unnecessarily copy their data to the heap. But this one is harder to avoid.

[nnethercote]

@bors r+

[bors]

📌 Commit 8c7a8a6 has been approved by nnethercote

[bors]

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

[bors]

⌛ Testing commit 8c7a8a6 with merge 4d223ce55315a94f18f16c53e665b64082c3025b...

[pietroalbini]

@bors treeclosed=1000

Need to fix another thing in the bors setup.

[pietroalbini]

@bors treeclosed- retry

[bors]

⌛ Testing commit 8c7a8a6 with merge f90e617...

[bors]

☀️ Test successful - checks-actions, checks-azure
Approved by: nnethercote
Pushing f90e617 to master...

[Mark-Simulacrum]

Final results are as expected.