Fix concurrent large transfers by using dedicated channel #38

antrik · 2016-02-21T18:23:21Z

As evidenced by the newly added "concurrent_senders" test case, the
previous approach never actually worked with concurrent large transfers:
when trying to "push back" packets to the "receiver" FD of the socket
pair, they do not show up on the "receiver" FD again, but rather the
"sender" FD.

This problem can be avoided entirely by automatically creating a
dedicated channel for each large transfer, so the followup fragments can
all be received in order, without interleaving with fragments from other
senders.

The first fragment of each large transfer is still sent through the main
channel, along with an FD for the dedicated channel; the remaining
fragments are then sent through the dedicated channel.

This approach adds a slight overhead for fragmented transfers when there
is no actual concurrency -- though it is way more efficient when actual
concurrency occurs than the push-back approach would have been...

With the dedicated channel, the follow-up fragments do not really need
the fragmentation headers anymore; and the header for the initial
fragment could be simplified as well. However, to keep changes minimal,
this commit doesn't change the header yet -- this is left for later
improvements.

antrik · 2016-02-21T18:26:17Z

According to @glennw this fixes hangs in WebRender.

emilio · 2016-02-21T20:19:29Z

platform/linux/mod.rs

-                iov_len: data_buffer.len() as size_t,
-            };
+                // Put this on the heap so address remains stable across function return.
+                let mut iovec = Box::new(iovec {


Isn't this relying on the iovec not being freed? (so we're either using it after free or leaking it?)

I don't know how critical it is, but maybe you could prevent this allocation, and the hackish iovec return passing an uninitialized &mut iovec to the closure.

I tried this too; however, it can't actually be passed uninitialised as far as I can see (the compiler complains) -- so in the end it looked uglier to me than the Box approach.

(Especially since in truth the iovec field is actually a variable-sized array... Which I intend to make explicit in one of the follow-up commits.)

There is std::mem::uninitialized() that allows you to do that :P

emilio · 2016-02-21T20:19:50Z

platform/linux/mod.rs

-                msg_flags: 0,
+                // Be sure to always return iovec -- whether the caller uses it or not --
+                // to prevent premature deallocation!
+                (msghdr, iovec)


Whoops, forget that, just read this line.

emilio · 2016-02-21T20:25:01Z

platform/linux/mod.rs

-                *cmsg_padding_ptr = *DEV_NULL;
-                cmsg_padding_ptr = cmsg_padding_ptr.offset(1);
-            }
+            let construct_header = |channels: &Vec<UnixChannel>, data_buffer: &Vec<u8>| {


nit: You can prevent the extra indirection using &[UnixChannel] instead of &Vec<UnixChannel>, and &[u8] instead of &Vec<u8>.

I wonder whether this would improve readability and/or performance?

(BTW, I intend to make major changes to this code in further iterations -- so probably not worthwhile to spend too much time polishing details... But I can change it of course if it's generally considered better style.)

I think it improves both (although performance-wise it's probably optimized away in release). It's fine for me if you don't change it just yet though.

OK -- being a newbie at Rust, I'll just trust you on this :-)

emilio · 2016-02-21T20:32:38Z

platform/linux/mod.rs

@@ -691,21 +709,19 @@ fn recv(fd: c_int, blocking_mode: BlockingMode)
            return Ok((main_data_buffer, channels, shared_memory_regions))
        }

-        // Reassemble fragments.
-        let mut leftover_fragments = Vec::new();
+        // Reassomble fragments.


nit: Reassemble

Whoops... Not sure how this slipped through. I usually reread my diffs thoroughly to prevent this kind of fail :-(

emilio · 2016-02-21T20:39:55Z

If I understand this well, this works by creating an extra dedicated channel at send, and receiving the dedicated channel receiver at recv time, and receiving leftovers through the dedicated channel.

I don't know how performant is it, but at least it seems correct :)

FWIW, all the tests are passing locally for me in both debug and release mode.

r? @pcwalton

emilio · 2016-02-21T20:43:59Z

@antrik Do you know if the max message size received in a single fragment is well defined?

Could the extra channel creation be prevented with small messages?

I guess it's not, but...

antrik · 2016-02-21T21:00:14Z

@ecoal95 check out the rest of the function: fragmentation (and creation of the dedicated channel) is only done if sending in one piece failed. For small messages, this change shouldn't introduce any overhead whatsoever.

emilio · 2016-02-21T21:06:18Z

@antrik Whoops, sorry, that's true (didn't read thoroughly, and missed the early return Ok(())).

Nice then! :)

antrik · 2016-02-21T21:19:59Z

Fixed the nits -- and also removed the now unused UnixCmsg.send() method, which I forgot to do earlier...

pcwalton · 2016-02-22T21:08:57Z

Looks good other than one stylistic nit. Thanks so much for looking into this!

antrik · 2016-02-23T21:55:24Z

That was the other major stylistic doubt I had... ;-)

The only variable it closes over is shared_memory_regions. I considered passing it explicitly too for more uniformity... And I also tried declaring it as a "regular" function. It felt a bit awkward because of the additional type annotations needed with that syntax -- but I'm not really opposed to it.

However, if I understand you right, you would actually want that as an entirely separate function rather than a nested one? I'd rather not do that: this code is tightly coupled; I'm sure it will feel awkward when I will be making further changes to it later on...

pcwalton · 2016-02-24T00:50:55Z

OK, I don't really care too much either way. I think I'd prefer a nested function if it isn't too much trouble.


        Fix concurrent large transfers by using dedicated channel

As evidenced by the newly added "concurrent_senders" test case, the previous approach never actually worked with concurrent large transfers: when trying to "push back" packets to the "receiver" FD of the socket pair, they do not show up on the "receiver" FD again, but rather the "sender" FD. This problem can be avoided entirely by automatically creating a dedicated channel for each large transfer, so the followup fragments can all be received in order, without interleaving with fragments from other senders. The first fragment of each large transfer is still sent through the main channel, along with an FD for the dedicated channel; the remaining fragments are then sent through the dedicated channel. This approach adds a slight overhead for fragmented transfers when there is no actual concurrency -- though it is way more efficient when actual concurrency occurs than the push-back approach would have been... With the dedicated channel, the follow-up fragments do not really need the fragmentation headers anymore; and the header for the initial fragment could be simplified as well. However, to keep changes minimal, this commit doesn't change the header yet -- this is left for later improvements.

antrik · 2016-02-24T18:51:09Z

Turned it into a "regular" nested function now.

pcwalton · 2016-02-24T19:31:47Z

@bors-servo: r+

bors-servo · 2016-02-24T19:31:49Z

📌 Commit ce7c296 has been approved by pcwalton

bors-servo · 2016-02-24T19:31:49Z

⌛ Testing commit ce7c296 with merge 8bd8d3e...


        Auto merge of #38 - antrik:fix-concurrent-send, r=pcwalton

Fix concurrent large transfers by using dedicated channel As evidenced by the newly added "concurrent_senders" test case, the previous approach never actually worked with concurrent large transfers: when trying to "push back" packets to the "receiver" FD of the socket pair, they do not show up on the "receiver" FD again, but rather the "sender" FD. This problem can be avoided entirely by automatically creating a dedicated channel for each large transfer, so the followup fragments can all be received in order, without interleaving with fragments from other senders. The first fragment of each large transfer is still sent through the main channel, along with an FD for the dedicated channel; the remaining fragments are then sent through the dedicated channel. This approach adds a slight overhead for fragmented transfers when there is no actual concurrency -- though it is way more efficient when actual concurrency occurs than the push-back approach would have been... With the dedicated channel, the follow-up fragments do not really need the fragmentation headers anymore; and the header for the initial fragment could be simplified as well. However, to keep changes minimal, this commit doesn't change the header yet -- this is left for later improvements.

antrik · 2016-02-24T19:35:00Z

Weird: it turns out that this change actually slightly improves performance even in the non-concurrent case...

bors-servo · 2016-02-24T19:48:17Z

☀️ Test successful - travis

emilio reviewed Feb 21, 2016
View changes

platform/linux/mod.rs

msg_flags: 0,

// Be sure to always return iovec -- whether the caller uses it or not --

// to prevent premature deallocation!

(msghdr, iovec)

This comment has been minimized.

Sign in to view

emilio Feb 21, 2016
Member

Whoops, forget that, just read this line.

emilio reviewed Feb 21, 2016
View changes

antrik force-pushed the antrik:fix-concurrent-send branch from 813c62d to 0bf557e Feb 21, 2016

antrik force-pushed the antrik:fix-concurrent-send branch from 0bf557e to ce7c296 Feb 24, 2016

servo / ipc-channel

Fix concurrent large transfers by using dedicated channel #38

Fix concurrent large transfers by using dedicated channel #38

antrik commented Feb 21, 2016

antrik commented Feb 21, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio commented Feb 21, 2016

emilio commented Feb 21, 2016

antrik commented Feb 21, 2016

emilio commented Feb 21, 2016

antrik commented Feb 21, 2016

pcwalton commented Feb 22, 2016

antrik commented Feb 23, 2016

pcwalton commented Feb 24, 2016

antrik commented Feb 24, 2016

pcwalton commented Feb 24, 2016

bors-servo commented Feb 24, 2016

bors-servo commented Feb 24, 2016

antrik commented Feb 24, 2016

bors-servo commented Feb 24, 2016

servo / ipc-channel

Join GitHub today

Fix concurrent large transfers by using dedicated channel #38

Fix concurrent large transfers by using dedicated channel #38

Conversation

antrik commented Feb 21, 2016

antrik commented Feb 21, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio commented Feb 21, 2016

emilio commented Feb 21, 2016

antrik commented Feb 21, 2016

emilio commented Feb 21, 2016

antrik commented Feb 21, 2016

pcwalton commented Feb 22, 2016

antrik commented Feb 23, 2016

pcwalton commented Feb 24, 2016

antrik commented Feb 24, 2016

pcwalton commented Feb 24, 2016

bors-servo commented Feb 24, 2016

bors-servo commented Feb 24, 2016

antrik commented Feb 24, 2016

bors-servo commented Feb 24, 2016