Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(iroh-blobs): Add tokio::io::Buf{Reader,Writer} to iroh_blobs::{get,provider} #2609

Closed
wants to merge 2 commits into from

Conversation

matheus23
Copy link
Contributor

Description

While trying to diagnose performance differences between the dig/quinn11 branch and main, I've stumbled upon this performance improvement for blob transfer.

This basically just introduces a tokio::io::BufReader to iroh_blobs::get and a tokio::io::BufWriter to iroh_blobs::provider.

Breaking Changes

None, all the types for this are internal.

Notes & open questions

  • What should the buffer size be?
    In my tests locally, anything above ~256KiB gave me diminishing returns, but values like 128KiB were still much worse. The default value for tokio::io::BufWriter is 8KiB, though, so significantly lower. What do people think?
  • Why is this faster?
    Usually these buffered readers help when the writes are small. I haven't looked at it in detail, but I could see how outboard data could consist of a bunch of smaller writes. Or perhaps there's a bunch of frame size u64s we're writing? Not sure.
  • Is iroh_blobs::protocol the right place for putting the DEFAULT_BUFFER_CAPACITY constant? It's one of the few modules which is common between the get and provide sides. FWIW, the constant is not exposed.
  • I'm interested in seeing the netsim numbers.

Change checklist

  • Self-review.
  • [ ] Documentation updates following the style guide, if relevant.
  • [ ] Tests if relevant.
  • All breaking changes documented.

@matheus23 matheus23 self-assigned this Aug 9, 2024
Copy link

github-actions bot commented Aug 9, 2024

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2609/docs/iroh/

Last updated: 2024-08-09T13:16:25Z

@matheus23
Copy link
Contributor Author

netsim test results
--- netsim-main-sorted.json	2024-08-09 15:53:57.758808998 +0200
+++ netsim-buf-perf-sorted.json	2024-08-09 15:53:45.789555261 +0200
@@ -2,201 +2,201 @@
   {
     "name": "time",
     "tag": "1_to_1.setup",
-    "value": 3.84
+    "value": 4.550000000000001
   },
   {
     "name": "time",
     "tag": "1_to_1.total",
-    "value": 7.33
+    "value": 8.08
   },
   {
     "name": "time",
     "tag": "1_to_1.transfer",
-    "value": 3.49
+    "value": 3.53
   },
   {
     "name": "time",
     "tag": "1_to_10.setup",
-    "value": 2.6499999999999986
+    "value": 3.1099999999999994
   },
   {
     "name": "time",
     "tag": "1_to_10.total",
-    "value": 18.4
+    "value": 16.31
   },
   {
     "name": "time",
     "tag": "1_to_10.transfer",
-    "value": 15.75
+    "value": 13.2
   },
   {
     "name": "reported_throughput_gbps",
     "tag": "1_to_10",
-    "value": 5.46
+    "value": 6.56
   },
   {
     "name": "throughput_gbps",
     "tag": "1_to_10",
-    "value": 4.67
+    "value": 5.28
   },
   {
     "name": "reported_throughput_gbps",
     "tag": "1_to_1",
-    "value": 2.46
+    "value": 2.43
   },
   {
     "name": "throughput_gbps",
     "tag": "1_to_1",
-    "value": 1.17
+    "value": 1.06
   },
   {
     "name": "time",
     "tag": "1_to_3.setup",
-    "value": 4.7700000000000005
+    "value": 4.289999999999999
   },
   {
     "name": "time",
     "tag": "1_to_3.total",
-    "value": 9.16
+    "value": 8.36
   },
   {
     "name": "time",
     "tag": "1_to_3.transfer",
-    "value": 4.39
+    "value": 4.07
   },
   {
     "name": "reported_throughput_gbps",
     "tag": "1_to_3",
-    "value": 5.87
+    "value": 6.33
   },
   {
     "name": "throughput_gbps",
     "tag": "1_to_3",
-    "value": 2.82
+    "value": 3.08
   },
   {
     "name": "time",
     "tag": "1_to_5.setup",
-    "value": 3.5600000000000005
+    "value": 3.570000000000001
   },
   {
     "name": "time",
     "tag": "1_to_5.total",
-    "value": 11.07
+    "value": 9.71
   },
   {
     "name": "time",
     "tag": "1_to_5.transfer",
-    "value": 7.51
+    "value": 6.14
   },
   {
     "name": "reported_throughput_gbps",
     "tag": "1_to_5",
-    "value": 5.74
+    "value": 7.02
   },
   {
     "name": "throughput_gbps",
     "tag": "1_to_5",
-    "value": 3.89
+    "value": 4.43
   },
   {
     "name": "time",
     "tag": "2_to_10.setup",
-    "value": 3.4299999999999997
+    "value": 3.7699999999999996
   },
   {
     "name": "time",
     "tag": "2_to_10.total",
-    "value": 12.2
+    "value": 12.82
   },
   {
     "name": "time",
     "tag": "2_to_10.transfer",
-    "value": 8.77
+    "value": 9.05
   },
   {
     "name": "reported_throughput_gbps",
     "tag": "2_to_10",
-    "value": 9.99
+    "value": 9.76
   },
   {
     "name": "throughput_gbps",
     "tag": "2_to_10",
-    "value": 7.08
+    "value": 6.74
   },
   {
     "name": "time",
     "tag": "2_to_2.setup",
-    "value": 3.91
+    "value": 4.29
   },
   {
     "name": "time",
     "tag": "2_to_2.total",
-    "value": 7.57
+    "value": 7.87
   },
   {
     "name": "time",
     "tag": "2_to_2.transfer",
-    "value": 3.66
+    "value": 3.58
   },
   {
     "name": "reported_throughput_gbps",
     "tag": "2_to_2",
-    "value": 4.7
+    "value": 4.8
   },
   {
     "name": "throughput_gbps",
     "tag": "2_to_2",
-    "value": 2.27
+    "value": 2.18
   },
   {
     "name": "time",
     "tag": "2_to_4.setup",
-    "value": 4.250000000000001
+    "value": 4.17
   },
   {
     "name": "time",
     "tag": "2_to_4.total",
-    "value": 8.31
+    "value": 8.41
   },
   {
     "name": "time",
     "tag": "2_to_4.transfer",
-    "value": 4.06
+    "value": 4.24
   },
   {
     "name": "reported_throughput_gbps",
     "tag": "2_to_4",
-    "value": 8.5
+    "value": 8.11
   },
   {
     "name": "throughput_gbps",
     "tag": "2_to_4",
-    "value": 4.15
+    "value": 4.09
   },
   {
     "name": "time",
     "tag": "2_to_6.setup",
-    "value": 3.5
+    "value": 3.8400000000000007
   },
   {
     "name": "time",
     "tag": "2_to_6.total",
-    "value": 9.49
+    "value": 9.3
   },
   {
     "name": "time",
     "tag": "2_to_6.transfer",
-    "value": 5.99
+    "value": 5.46
   },
   {
     "name": "reported_throughput_gbps",
     "tag": "2_to_6",
-    "value": 8.62
+    "value": 9.49
   },
   {
     "name": "throughput_gbps",
     "tag": "2_to_6",
-    "value": 5.43
+    "value": 5.54
   }
 ]

Inconclusive I'd say.
It did help improve performance for me for 1-to-1 transfer pretty reliably, but it doesn't seem that's the case in general for netsim.

@matheus23 matheus23 marked this pull request as draft August 9, 2024 13:57
@rklaehn
Copy link
Contributor

rklaehn commented Aug 12, 2024

I played around with buffering way back when I wrote all this, but could not get sufficiently large improvements to make it seem worth it.

It did help improve performance for me for 1-to-1 transfer pretty reliably, but it doesn't seem that's the case in general for netsim.

What kind of perf improvements did you see? Back when I wrote this it was always noticeable but not big enough to make it a clear win.

In any case I think it would be better to do buffering in iroh-io. See n0-computer/iroh-io#2

Regarding buffer size: It would have to be at least 16 KiB, plus some extra bytes for the tree nodes. Minimally you want all the small writes before a chunk of data to be combined with said chunk of data.

@matheus23
Copy link
Contributor Author

What kind of perf improvements did you see? Back when I wrote this it was always noticeable but not big enough to make it a clear win.

133 MiB/s vs. 216 MiB/s.

@matheus23
Copy link
Contributor Author

Closing this, as it doesn't seem to be a reliable way of improving performance.

@matheus23 matheus23 closed this Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants