Implment Request Stream for UDP Tracker #873

da2ce7 · 2024-05-18T14:47:18Z

This pull request improves the UDP tracker server.rs request processing logic; now using a tokio::stream for getting the requests for the ring buffer.

I hope this pull request will improve the performance of the tracker in multi-core setups.

@josecelano This code will require a special code-review.

codecov · 2024-05-19T12:12:02Z

Codecov Report

Attention: Patch coverage is 79.38671% with 121 lines in your changes missing coverage. Please review.

Project coverage is 78.75%. Comparing base (a6054f1) to head (b6c7fe2).
Report is 14 commits behind head on develop.

❗ Current head b6c7fe2 differs from pull request most recent head 84cc1a1

Please upload reports for the commit 84cc1a1 to get more accurate results.

Files	Patch %	Lines
src/servers/udp/server.rs	79.39%	115 Missing ⚠️
src/shared/bit_torrent/tracker/udp/client.rs	76.92%	6 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #873      +/-   ##
===========================================
+ Coverage    78.53%   78.75%   +0.21%     
===========================================
  Files          171      170       -1     
  Lines         9458     9873     +415     
===========================================
+ Hits          7428     7775     +347     
- Misses        2030     2098      +68

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

da2ce7 · 2024-05-19T14:34:49Z

@josecelano This pull request is ready for review.

josecelano

Hi @da2ce7 I haven't reviewed the code yet but I have executed the UDP load lest:

Current version

MAX

Requests out: 435973.31/second
Responses in: 392346.07/second
  - Connect responses:  194332.01
  - Announce responses: 194108.16
  - Scrape responses:   3905.90
  - Error responses:    0.00
Peers per announce response: 0.00
Announce responses per info hash:
  - p10: 1
  - p25: 1
  - p50: 1
  - p75: 1
  - p90: 2
  - p95: 3
  - p99: 105
  - p99.9: 315
  - p100: 391

MIN

Requests out: 371407.92/second
Responses in: 334267.17/second
  - Connect responses:  165563.03
  - Announce responses: 165413.11
  - Scrape responses:   3291.02
  - Error responses:    0.00
Peers per announce response: 0.00
Announce responses per info hash:
  - p10: 1
  - p25: 1
  - p50: 1
  - p75: 1
  - p90: 2
  - p95: 3
  - p99: 104
  - p99.9: 271
  - p100: 339

This version

MAX

Requests out: 157926.53/second
Responses in: 142133.56/second
  - Connect responses:  70206.70
  - Announce responses: 70498.45
  - Scrape responses:   1428.41
  - Error responses:    0.00
Peers per announce response: 0.00
Announce responses per info hash:
  - p10: 1
  - p25: 1
  - p50: 1
  - p75: 1
  - p90: 2
  - p95: 10
  - p99: 78
  - p99.9: 125
  - p100: 156s

MIN

Requests out: 79844.49/second
Responses in: 71860.52/second
  - Connect responses:  35539.87
  - Announce responses: 35614.59
  - Scrape responses:   706.05
  - Error responses:    0.00
Peers per announce response: 0.00
Announce responses per info hash:
  - p10: 1
  - p25: 1
  - p50: 1
  - p75: 1
  - p90: 4
  - p95: 16
  - p99: 49
  - p99.9: 68
  - p100: 88

josecelano · 2024-06-03T09:06:31Z

Hi @da2ce7 I found this challenge:

https://mas-bandwidth.com/writing-highly-scalable-backends-in-udp/

And solutions:

da2ce7 · 2024-06-19T13:48:43Z

@josecelano reimplmented with a much simpler, (and not-slower?), version. :)

josecelano · 2024-06-19T13:54:44Z

@josecelano reimplmented with a much simpler, (and not-slower?), version. :)

Hey @da2ce7 cool! I will review it tomorrow after the meeting. I'm almost finished with the configuration refactorings.

josecelano · 2024-06-20T16:48:01Z

@josecelano reimplmented with a much simpler, (and not-slower?), version. :)

Hi @da2ce7, this version is even better than the previous one 🥇 !!.

At least with this concrete performance test that I'm running. I will review the code.

I have to double-check that the current version has the same result. The computer is exactly the same. However, we updated the BIOS. I guess that should not affect it.

This version

MAX

Requests out: 478727.77/second
Responses in: 431048.72/second
  - Connect responses:  213289.40
  - Announce responses: 213400.69
  - Scrape responses:   4358.63
  - Error responses:    0.00
Peers per announce response: 0.00
Announce responses per info hash:
  - p10: 1
  - p25: 1
  - p50: 1
  - p75: 1
  - p90: 2
  - p95: 3
  - p99: 104
  - p99.9: 343
  - p100: 431

MIN

Requests out: 433379.95/second
Responses in: 393697.26/second
  - Connect responses:  194904.16
  - Announce responses: 194860.20
  - Scrape responses:   3932.91
  - Error responses:    0.00
Peers per announce response: 0.00
Announce responses per info hash:
  - p10: 1
  - p25: 1
  - p50: 1
  - p75: 1
  - p90: 2
  - p95: 3
  - p99: 105
  - p99.9: 313
  - p100: 391

josecelano · 2024-06-20T18:04:11Z

The current version (target branch) is slightly better (the max) but, on average, much slower than the new one (this PR).

The new MIN is almost as good as the previous MAX. I will run again the whole benchmarking.

NEW MIN

Requests out: 433379.95/second
Responses in: 393697.26/second

CURRENT MAX

Requests out: 448806.70/second
Responses in: 403873.92/second

Current version

MAX

Requests out: 448806.70/second
Responses in: 403873.92/second
  - Connect responses:  200015.65
  - Announce responses: 199875.73
  - Scrape responses:   3982.54
  - Error responses:    0.00
Peers per announce response: 0.00
Announce responses per info hash:
  - p10: 1
  - p25: 1
  - p50: 1
  - p75: 1
  - p90: 2
  - p95: 3
  - p99: 104
  - p99.9: 323
  - p100: 403

MIN

Requests out: 381837.70/second
Responses in: 343653.57/second
  - Connect responses:  170156.07
  - Announce responses: 170132.54
  - Scrape responses:   3364.96
  - Error responses:    0.00
Peers per announce response: 0.00
Announce responses per info hash:
  - p10: 1
  - p25: 1
  - p50: 1
  - p75: 1
  - p90: 2
  - p95: 3
  - p99: 105
  - p99.9: 277
  - p100: 355

josecelano

Hi @da2ce7, I think this improves two important things:

It removes the mess of "processor" functions.
It introduces some abstractions (Receiver, BoundSocket, ...) that make the code more straightforward and clear.

And it improves significantly the performance, so I think we should merge it. All my comments are only suggestions to potentially improve the readability, and they can be done later.

josecelano · 2024-06-24T07:09:37Z

src/servers/udp/server.rs

-        });
+        let halt_task = tokio::task::spawn(shutdown_signal_with_message(
+            rx_halt,
+            format!("Halting Http Service Bound to Socket: {bind_to}"),


Hi @da2ce7 I think the message should be "Halting UDP Service Bound to Socket: {bind_to}"

josecelano · 2024-06-24T07:16:54Z

src/servers/udp/server.rs

+        // note: this log message is parsed by our container. i.e:
+        //
+        // `[UDP TRACKER][INFO] Starting on: udp://`
+        //
+        tracing::info!(target: "UDP TRACKER", "Starting on: {local_addr}");


Hi @da2ce7, maybe we could make this dependency explicit. We could move the const from:

https://github.com/torrust/torrust-tracker/blob/develop/src/console/ci/e2e/logs_parser.rs#L5-L8

https://github.com/torrust/torrust-tracker/blob/develop/src/console/ci/e2e/logs_parser.rs#L67-L69

to the UDP server mod. Ideally, we should also use a field in the log now that we are using tracing. But that could be implemented in a new PR.

josecelano · 2024-06-24T07:31:58Z

src/servers/udp/server.rs

@@ -235,6 +234,84 @@ impl Drop for ActiveRequests {
    }
 }

+/// Wrapper for Tokio [`UdpSocket`][`tokio::net::UdpSocket`] that is bound to a particular socket.
+struct Socket {


Hi @da2ce7, I would call it BoundSocket and add a local_addr() method.

josecelano · 2024-06-24T07:36:49Z

src/servers/udp/server.rs

+        let direct = Receiver {
+            socket,
+            tracker,
+            data: RefCell::new([0; MAX_PACKET_SIZE]),
+        };


Hi @da2ce7, why did you call this "direct"?

@josecelano yes, this is a poor name, it is from a experiment where the requests were 'directly' made...

Hi, @da2ce7 ok.

josecelano · 2024-06-24T07:44:37Z

src/servers/udp/server.rs

+                Some(Ok(tokio::task::spawn(Udp::process_request(
+                    request,
+                    self.tracker.clone(),
+                    self.socket.clone(),
+                ))
+                .abort_handle()))


@da2ce7 I like this trait implementation. However, I like the idea of having a more "pure" receiver that only returns the request. I would move this spawn to the main loop or maybe the Reciever could have another wrapper StreamProcessor or something like that that takes the requests and launches the worker. I think in the future, we could have a limited number of workers instead of spawning a new task, although I guess Tokio handles that.

src/servers/udp/server.rs

josecelano · 2024-06-24T07:57:25Z

ACK 84cc1a1

josecelano · 2024-06-24T09:16:22Z

Hi @da2ce7, just for the record, this PR behaves the same way as the current version. We have a limit of 50 active requests. When the server has more than 50 simultaneously, it starts aborting the oldest tasks. As far as I can see, this enabled a kind of indirect timeout. Maybe we should increase the buffer and set an explicit timeout to process UDP requests. I guess that would add an overhead, and in the end, the effect would be the same; some requests are aborted when the server is overloaded. The advantage of this solution is the timeout is somehow dynamic. Just out of curiosity, I've run the test load for 1 minute on my PC, and there were 6 aborted requests. We have to check that this is not affecting the benchmarking. I don't think so because the UDP load tests count the responses.

josecelano · 2024-06-25T09:53:30Z

Hi @da2ce7 I'm working on the refactorings I've suggested in this PR: #913

Now that I'm working on the code and not just reviewing it, I've noticed one thing. When we receive a new request, we immediately spawn a new task to handle it. However, we limit the number of active requests to 50. If we are already handling 50 requests and a new request comes in, we spawn a new task and remove the oldest tasks in the active requests buffer. Would not it make sense not to spawn new tasks if the active request buffer is already full?

We could have a "pending to activate requests" buffer in front of the active reqs buffer. When tasks in the active buffer finish we get more requests from the first requests buffer and spwan a new task to handle them.

Pros:

We don't waste more resources spawning tasks that will not be executed immediately.
We don't need to remove active requests (we don't need to make a place for new requests if the active reqs buffer is complete).

Cons:

We indirectly increase the timeout. This could be a good or a bad thing. It would be good if the timeout was too short, and bad if it was too long.

By the way, do you want to continue working on this PR? I can stop working on the new one , but I think it's a good thing to alternately work on the same to share the code ownership.

f06976e docs: update some UDP server comments (Jose Celano) 89bb735 refactor: reorganize UDP server mod (Jose Celano) c121bf2 refactor: rename UDP server types (Jose Celano) 336e0e6 refactor: reorganize mod to extract new submods (Jose Celano) 61fb4b2 refactor: move active requests logic to ActiveRequest type (Jose Celano) 35b6c84 refactor: simplify UDP server receiver (Jose Celano) a5e2baf refactor: extract method (Jose Celano) b4b4515 refactor: extract const for logging targets (Jose Celano) 0388e1d refactor: extract consts for logging targets (Jose Celano) 16ae4fd refactor: rename vars and extract constructor (Jose Celano) 7ff0cd2 refactor: rename var (Jose Celano) 0e3678d refactor: rename Socket to BoundSocket and fix format errors" (Jose Celano) 9b3b75b fix: log message (Jose Celano) Pull request description: This PR only includes some minor changes I've proposed in this [PR](#873), and some refactorings. ACKs for top commit: josecelano: ACK f06976e Tree-SHA512: a7a4ea14077c2ce6df0b80b222952d0c6c6588f1df50f78d01198ea0ab12ce3ca74923caed6601994e955880e2afe3d9432f87ae74f383362fae452e367ad359

da2ce7 requested a review from josecelano May 18, 2024 14:47

da2ce7 had a problem deploying to coverage May 18, 2024 14:47 — with GitHub Actions Error

da2ce7 force-pushed the 20240518_udp_request_stream branch from c58f073 to 3483324 Compare May 18, 2024 14:59

da2ce7 had a problem deploying to coverage May 18, 2024 14:59 — with GitHub Actions Error

da2ce7 force-pushed the 20240518_udp_request_stream branch from 3483324 to 1b481be Compare May 18, 2024 15:02

da2ce7 had a problem deploying to coverage May 18, 2024 15:02 — with GitHub Actions Error

da2ce7 force-pushed the 20240518_udp_request_stream branch from 1b481be to dce4c4b Compare May 18, 2024 15:37

da2ce7 had a problem deploying to coverage May 18, 2024 15:37 — with GitHub Actions Error

da2ce7 had a problem deploying to coverage May 18, 2024 18:11 — with GitHub Actions Failure

da2ce7 force-pushed the 20240518_udp_request_stream branch from 438dae2 to 401925f Compare May 19, 2024 12:04

da2ce7 temporarily deployed to coverage May 19, 2024 12:04 — with GitHub Actions Inactive

da2ce7 force-pushed the 20240518_udp_request_stream branch from 401925f to e623a55 Compare May 19, 2024 12:30

da2ce7 temporarily deployed to coverage May 19, 2024 12:30 — with GitHub Actions Inactive

da2ce7 marked this pull request as ready for review May 19, 2024 14:34

josecelano reviewed May 20, 2024

View reviewed changes

josecelano added the Needs Rebase Base Branch has Incompatibilities label May 20, 2024

da2ce7 force-pushed the 20240518_udp_request_stream branch from e623a55 to b6c7fe2 Compare May 21, 2024 04:38

da2ce7 temporarily deployed to coverage May 21, 2024 04:38 — with GitHub Actions Inactive

da2ce7 removed the Needs Rebase Base Branch has Incompatibilities label May 21, 2024

josecelano added the Needs Rebase Base Branch has Incompatibilities label May 21, 2024

da2ce7 force-pushed the 20240518_udp_request_stream branch from b6c7fe2 to dc12890 Compare June 19, 2024 13:47

da2ce7 had a problem deploying to coverage June 19, 2024 13:47 — with GitHub Actions Failure

da2ce7 removed the Needs Rebase Base Branch has Incompatibilities label Jun 19, 2024

da2ce7 force-pushed the 20240518_udp_request_stream branch from dc12890 to 7df1201 Compare June 19, 2024 13:49

da2ce7 had a problem deploying to coverage June 19, 2024 13:49 — with GitHub Actions Failure

da2ce7 force-pushed the 20240518_udp_request_stream branch from 7df1201 to 584a38a Compare June 19, 2024 13:53

da2ce7 had a problem deploying to coverage June 19, 2024 13:53 — with GitHub Actions Failure

da2ce7 force-pushed the 20240518_udp_request_stream branch from 584a38a to e663c9c Compare June 19, 2024 13:55

da2ce7 had a problem deploying to coverage June 19, 2024 13:55 — with GitHub Actions Failure

da2ce7 force-pushed the 20240518_udp_request_stream branch from e663c9c to 4c67a83 Compare June 19, 2024 13:58

da2ce7 had a problem deploying to coverage June 19, 2024 13:58 — with GitHub Actions Failure

dev: use stream for udp requests

84cc1a1

da2ce7 force-pushed the 20240518_udp_request_stream branch from 4c67a83 to 84cc1a1 Compare June 19, 2024 14:01

da2ce7 had a problem deploying to coverage June 19, 2024 14:01 — with GitHub Actions Failure

josecelano self-requested a review June 21, 2024 07:07

josecelano mentioned this pull request Jun 21, 2024

Add a workflow to run a basic benchmark for the UDP tracker #909

Open

josecelano approved these changes Jun 24, 2024

View reviewed changes

josecelano self-assigned this Jun 24, 2024

josecelano added the Code Cleanup / Refactoring Tidying and Making Neat label Jun 24, 2024

josecelano merged commit b7bcd96 into torrust:develop Jun 25, 2024
10 of 12 checks passed

josecelano mentioned this pull request Jun 25, 2024

Minor UDP server refactorings #913

Merged

josecelano mentioned this pull request Jun 26, 2024

UDP server: alternative implementation to avoid aborting too many requests #918

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implment Request Stream for UDP Tracker #873

Implment Request Stream for UDP Tracker #873

da2ce7 commented May 18, 2024 •

edited

Loading

codecov bot commented May 19, 2024 •

edited

Loading

da2ce7 commented May 19, 2024

josecelano left a comment

josecelano commented Jun 3, 2024

da2ce7 commented Jun 19, 2024

josecelano commented Jun 19, 2024

josecelano commented Jun 20, 2024 •

edited

Loading

josecelano commented Jun 20, 2024

josecelano left a comment

josecelano Jun 24, 2024

josecelano Jun 24, 2024

josecelano Jun 24, 2024

josecelano Jun 24, 2024

da2ce7 Jun 25, 2024

josecelano Jun 25, 2024

josecelano Jun 24, 2024

josecelano commented Jun 24, 2024

josecelano commented Jun 24, 2024 •

edited

Loading

josecelano commented Jun 25, 2024 •

edited

Loading

Implment Request Stream for UDP Tracker #873

Implment Request Stream for UDP Tracker #873

Conversation

da2ce7 commented May 18, 2024 • edited Loading

codecov bot commented May 19, 2024 • edited Loading

Codecov Report

da2ce7 commented May 19, 2024

josecelano left a comment

Choose a reason for hiding this comment

Current version

This version

josecelano commented Jun 3, 2024

da2ce7 commented Jun 19, 2024

josecelano commented Jun 19, 2024

josecelano commented Jun 20, 2024 • edited Loading

This version

josecelano commented Jun 20, 2024

Current version

josecelano left a comment

Choose a reason for hiding this comment

josecelano Jun 24, 2024

Choose a reason for hiding this comment

josecelano Jun 24, 2024

Choose a reason for hiding this comment

josecelano Jun 24, 2024

Choose a reason for hiding this comment

josecelano Jun 24, 2024

Choose a reason for hiding this comment

da2ce7 Jun 25, 2024

Choose a reason for hiding this comment

josecelano Jun 25, 2024

Choose a reason for hiding this comment

josecelano Jun 24, 2024

Choose a reason for hiding this comment

josecelano commented Jun 24, 2024

josecelano commented Jun 24, 2024 • edited Loading

josecelano commented Jun 25, 2024 • edited Loading

da2ce7 commented May 18, 2024 •

edited

Loading

codecov bot commented May 19, 2024 •

edited

Loading

josecelano commented Jun 20, 2024 •

edited

Loading

josecelano commented Jun 24, 2024 •

edited

Loading

josecelano commented Jun 25, 2024 •

edited

Loading