Finally introduce sane unified scheduler shutdown by ryoqun · Pull Request #5866 · anza-xyz/agave

ryoqun · 2025-04-17T14:34:26Z

Problem

I was lazy. There was wip shutdown impl buried in an ancient pr..

And that wip work is now well integrated with bp scheduler code-path.

Summary of Changes

An alternative approach to #5826

fyi: salvaged from #593 by porting it to #3946 and force-pushed to this pr by extracting from there.

ryoqun · 2025-04-17T14:44:49Z


+impl Drop for BankForks {
+    fn drop(&mut self) {
+        error!("BankForks::drop(): successfully dropped");


note to self: now that, ProgramCache's BankForks circular-referenc is gone. maybe i can wire prepare_to_drop from here to exercise the shutdown code-path (esp in tests!)

this is done: 163015a

apfitzge

I'm a bit biased toward my solution in #5826 because I think have the strong circular references adds quite a bit of room for us to introduce a similar bug in the future whereas weak references make that harder to do.

However I think most of these changes make sense in addition to my PR, and give us stronger confidence in shutdown logic. AFAICT these two PRs are not conflicting, please let me know if you see a reason why this would not be the case.

apfitzge · 2025-04-18T14:03:15Z

        drop(self.cluster_info);

-        self.poh_service.join().expect("poh_service");
+        macro_rules! join_then_log {


This seems like a separate change to assist in debugging thread-joining issues. Probably a good change to make, but not related to fixing this issue afiact?

yeah. another piggy-bucked salvation from the ancient pr #593 by procrastinating me.

apfitzge · 2025-04-18T14:13:20Z

        bank
    }

+    pub fn prepare_to_drop(&mut self) {


why not just do a custom drop?

this is done: #5866 (comment)

codecov-commenter · 2025-04-18T15:30:47Z

Codecov Report

Attention: Patch coverage is 69.04762% with 39 lines in your changes missing coverage. Please review.

Project coverage is 83.2%. Comparing base (e9ac77f) to head (63dc14c).
Report is 47 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #5866    +/-   ##
========================================
  Coverage    83.2%    83.2%            
========================================
  Files         853      853            
  Lines      375116   375214    +98     
========================================
+ Hits       312225   312330   +105     
+ Misses      62891    62884     -7

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ryoqun · 2025-04-21T15:46:14Z

I'm a bit biased toward my solution in #5826 because I think have the strong circular references adds quite a bit of room for us to introduce a similar bug in the future whereas weak references make that harder to do.

First of all, thanks for working on unified scheduler code.

To be clear, i have a strong preference regarding strong circular references and relaxing it by weak ones. but I'm not planning to push my idea to the team so hard.

So, if i cannot change your mind with this comment, I'm fine to merge #5826. it's written after good understanding of unified scheduler and it's indeed a good solution. I'm quite happy to see these kinds of prs from someone other than me.. :) And it reduces my work load for now. however, I might revisit it in the future, as i said at bottom of this wall of text (due to real perf reasons)... xD

Maybe, the difference of approaches in the two prs boils down to different coding stances.

I agree that a strong circular references should be avoided if possible and that it's error-prone. However, at the same time, i think this downside can be under control with strict coding of full of assert!()s and comments. Maybe, this stance is coming from my high-demanding assumption that code should be written with full and precise understanding of all of surrounding subsystems. ;)

Fyi, this bug is known (at least by me!) since start, and i still don't think local-cluster test (#5435) and production agave-validator binary is experiencing the reported shutdown problems directly due to these strong circular references. That's my justification i significantly lowered working on shipping this shutdown impl. For example, scheduler won't prevent BankFork from being Drop-ed as far as i run the local-cluster test locally on the base commit of this pr.

On the other hand, I agree weak circular references does indeed fix the pressing problem (if any!). However, i think it clouds system responsibilities. Namely, guarantee of ThreadManager and new_task_sender outlive relationship to others if upgrade() error condition (returned None) is squashed in some way. From my experience, tracking down bugs from these relaxed runtime invariant is very hard and time-consuming when this is happening outside unit tests.

Lastly, there's a rationale for strong circular references other than introduction of strict runtime invariant: performance. For new_task_sender, Weak::upgrade is similar to Arc::clone in terms of perf impact. It introduces another source of cache-line bouncing and memory indirection under many threads. and it's done per task, meaning quite large overhead (I don't have concrete synesthetic bench numbers off hand right now, though).

However I think most of these changes make sense in addition to my PR, and give us stronger confidence in shutdown logic. AFAICT these two PRs are not conflicting, please let me know if you see a reason why this would not be the case.

Yeah, we can mix the two technically. however, i think merging either one should be enough. I think we should stick to one philosophy and be consistent with it.

apfitzge · 2025-07-14T15:20:27Z

@ryoqun is this ready to review?

ryoqun · 2025-07-17T03:08:09Z

@ryoqun is this ready to review?

@apfitzge this should be ready by now. reworked for bp schedulers and simplified the impl so that core changes are now minimal.

apfitzge

minor comments on the unreachable.
would like to remove the additional contention on poh_recorder before merging

apfitzge · 2025-07-18T15:28:38Z

 impl BankingStageMonitor for DecisionMaker {
    fn status(&mut self) -> BankingStageStatus {
-        if matches!(
+        if self.poh_recorder.read().unwrap().is_exited.load(Relaxed) {


Would be good to just clone is_exited off of poh_recorder. It's already wrapped in an Arc, so we can avoid locking poh_recorder each time we check status.

nice suggestion!: a9916d3

anza-team · 2025-07-22T06:43:47Z

😱 New commits were pushed while the automerge label was present.

anza-team · 2025-07-22T09:25:31Z

😱 New commits were pushed while the automerge label was present.

ryoqun · 2025-07-22T09:29:32Z

fyi, i added 63dc14c after noticing ci delays across board...

anza-team · 2025-07-22T09:59:00Z

😱 New commits were pushed while the automerge label was present.

* Finally introduce sane unified scheduler shutdown * Avoid lock contentions on poh_recorder * Remove confusing redundant comment * Provide explicit msg to unreachable!()s * Minor edits * Improve ci stability with faster joining

ryoqun marked this pull request as draft April 17, 2025 14:34

mergify Bot added the need:merge-assist label Apr 17, 2025

ryoqun commented Apr 17, 2025

View reviewed changes

ryoqun requested a review from apfitzge April 17, 2025 14:47

apfitzge added the CI Pull Request is ready to enter CI label Apr 18, 2025

anza-team removed the CI Pull Request is ready to enter CI label Apr 18, 2025

apfitzge reviewed Apr 18, 2025

View reviewed changes

ryoqun requested a review from apfitzge April 22, 2025 01:58

ryoqun force-pushed the unified-scheduler-shutdown branch 2 times, most recently from f6f9acb to 903fd09 Compare July 17, 2025 03:03

ryoqun marked this pull request as ready for review July 17, 2025 03:08

ryoqun force-pushed the unified-scheduler-shutdown branch from 903fd09 to 0842d6a Compare July 17, 2025 04:44

Finally introduce sane unified scheduler shutdown

c943b74

ryoqun force-pushed the unified-scheduler-shutdown branch from 0842d6a to c943b74 Compare July 17, 2025 09:34

apfitzge requested changes Jul 18, 2025

View reviewed changes

ryoqun added 3 commits July 22, 2025 15:03

Avoid lock contentions on poh_recorder

a9916d3

Remove confusing redundant comment

c5b866c

Provide explicit msg to unreachable!()s

5509cde

ryoqun force-pushed the unified-scheduler-shutdown branch from a78c13b to 5509cde Compare July 22, 2025 06:03

ryoqun requested a review from apfitzge July 22, 2025 06:09

ryoqun added the automerge automerge Merge this Pull Request automatically once CI passes label Jul 22, 2025

anza-team removed the automerge automerge Merge this Pull Request automatically once CI passes label Jul 22, 2025

Minor edits

3e35d9f

ryoqun force-pushed the unified-scheduler-shutdown branch from bf31400 to 3e35d9f Compare July 22, 2025 06:45

ryoqun added the automerge automerge Merge this Pull Request automatically once CI passes label Jul 22, 2025

anza-team removed the automerge automerge Merge this Pull Request automatically once CI passes label Jul 22, 2025

ryoqun force-pushed the unified-scheduler-shutdown branch from 23ac0b4 to 2211a02 Compare July 22, 2025 09:28

ryoqun added the automerge automerge Merge this Pull Request automatically once CI passes label Jul 22, 2025

Improve ci stability with faster joining

63dc14c

ryoqun force-pushed the unified-scheduler-shutdown branch from 2211a02 to 63dc14c Compare July 22, 2025 09:58

anza-team removed the automerge automerge Merge this Pull Request automatically once CI passes label Jul 22, 2025

apfitzge approved these changes Jul 22, 2025

View reviewed changes

ryoqun added the automerge automerge Merge this Pull Request automatically once CI passes label Jul 22, 2025

apfitzge merged commit ddc4b56 into anza-xyz:master Jul 22, 2025
43 checks passed

Conversation

ryoqun commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apfitzge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ryoqun commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apfitzge commented Jul 14, 2025

Uh oh!

ryoqun commented Jul 17, 2025

Uh oh!

apfitzge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

anza-team commented Jul 22, 2025

Uh oh!

anza-team commented Jul 22, 2025

Uh oh!

ryoqun commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anza-team commented Jul 22, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ryoqun commented Apr 17, 2025 •

edited

Loading

codecov-commenter commented Apr 18, 2025 •

edited

Loading

ryoqun commented Apr 21, 2025 •

edited

Loading

ryoqun commented Jul 22, 2025 •

edited

Loading