Improve supervisor restart calculation

[Maria-12648430]

Before restarting a child, a supervisor must check if the restart limit is reached. This adds a penalty to the overall restart time, which should be kept low.

The current implementation does this check by traversing the list of restarts in order to filter out those that have expired. Then it essentially traverses the result list via length in order to check if it is over the intensity limit. This behavior is 2*O(n) (?), with n being the number of past restarts within the period.

This PR introduces two optimizations:

• it checks whether the restart limit is reached while it is traversing the restart list in order to remove expired restarts, thereby eliminating the need for an additional traversal via the call to length. Depending on the outcome, a restart is either allowed or disallowed. This behavior is O(n).

• it sidesteps the need to perform the step above by keeping a separate counter for restarts; as long as that counter is below the intensity value, it is safe to allow the restart, add the restart to the list, and increment the counter. This behavior is O(1).

  Only when the counter reaches the intensity limit, the actual number of restarts within the given period must be calculated via the step above; if the restart is allowed, the restart list is updated and the counter set to the according value.

  (Over time, this may lead to a large list of accumulated expired restarts being carried around. For this reason, the counter is limited not by the intensity value alone but rather by the minimum of the intensity value and a hardcoded limit. By gut feeling, I picked 1000)

[github-actions]

CT Test Results

    2 files     94 suites   34m 30s ⏱️
2 042 tests 1 994 ✅ 48 💤 0 ❌
2 351 runs  2 301 ✅ 50 💤 0 ❌

Results for commit 6b31837.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

• Complete CT logs (Download Logs)
• HTML Documentation (Download HTML Docs)
• Windows Installer

// Erlang/OTP Github Action Bot

[IngelaAndin]

New optimizations are way to dangerous to include so close to the release, you never know what timing bugs that might be revealed, so we are postponing this for OTP-28.

[IngelaAndin]

Looks good in the test, that is I can not see any new test failures that seems related to this. I will merge for OTP-28 so that we have plenty of time to test it further. I guess most other people, the us, will however not test it until the first release candidate is out.

[IngelaAndin]

@Maria-12648430 seems our systems where lagging a little :( Now I see failures for two test cases that are caused by this PR.

The error I get manifests as following in the application SASL test suite:

 === Location: [{release_handler_SUITE,upgrade_supervisor,[1664](http://otp.ericsson.se/product/internal/test/test_results/progress_28/2024_08_25/otp_28_sebastian_linux-gnu_x86_64_64-bit_jit_s4_kp_a1_no_prevent_overlapping_partitions_docker_111355_28/[email protected]_01.08.20/test.sasl_test.logs/run.2024-08-26_01.08.26/release_handler_suite.src.html#1664)},
              {test_server,ts_tc,1794},
              {test_server,run_test_case_eval1,1303},
              {test_server,run_test_case_eval,1235}]
=== === Reason: no match of right hand side value 
                 {state,{local,a_sup},
                        one_for_all,
                        {[a],
                         #{a =>
                               {child,<20453.100.0>,a,
                                      {a,start_link,[]},
                                      permanent,false,brutal_kill,worker,
                                      [a]}}},
                        undefined,4,3600,[],0,0,never,a_sup,[]}
  in function  release_handler_SUITE:upgrade_supervisor/1 (release_handler_SUITE.erl, line 1664)
  in call from test_server:ts_tc/3 (test_server.erl, line 1794)
  in call from test_server:run_test_case_eval1/6 (test_server.erl, line 1303)
  in call from test_server:run_test_case_eval/9 (test_server.erl, line 1235)

Could you start by seeing I you can recreate it in your test environment ?

[Maria-12648430]

Hi @IngelaAndin, I think I can look at it tomorrow or the day after, right now I'm on my mobile, ie not anywhere near a real computer ;) But it looks like the test tries to match on the supervisor state record, which got a new field with this PR.

[Maria-12648430]

Ah, yes, this is the failing line:

otp/lib/sasl/test/release_handler_SUITE.erl

Line 1664 in 695c729

{state,_,RestartStrategy,{[a],Db},_,_,_,_,_,_,_,_} = State,

Just adding another _ to the tuple should do the trick then.

[Maria-12648430]

@IngelaAndin ok, I got it sorted out. But you said two test cases were failing? What is the other one?

[IngelaAndin]

@Maria-12648430 Well same test is run in two groups (relative and absolute), so your fix should fix both tests as they are the same in this respect. Could you please make a PR that updates the test suite?

[Maria-12648430]

Could you please make a PR that updates the test suite?

Sure: #8758 🙂