RingBuffer contiguous memory optimization #187

astro · 2018-04-09T21:26:19Z

I've studied the code for a while and wondered why there is a check for capacity > 0 at this location. capacity doesn't change. I believe self.length was actually intended here.

In the RawSocket use by #186 that's needed otherwise PacketBuffer tends to get a stuck payload_ring (length=0 but read_at near capacity).

dlrobertson

The addition of the check for capacity > 0 was to ensure we didn't cause a divide by zero when a zero size buffer was given (which often occurs in tests). Why were the test strings changed?

astro · 2018-04-10T14:17:32Z

The test strings were changed to make the CI pass.

I didn't encounter any division by zero errors. Should I try to solve my problem in a different way?

whitequark · 2018-04-10T14:29:45Z

If you're changing tests "to make CI green" that means it's equally likely that there was a bug in code or a bug in tests or a bug in specs but you didn't distinguish these cases. Which one is it?

…

On April 10, 2018 10:17:36 PM GMT+08:00, Astro ***@***.***> wrote: The test strings were changed to make the CI pass. I didn't encounter any division by zero errors. Should I try to solve my problem in a different way? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #187 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

astro · 2018-04-10T14:35:07Z

I'm not 100% certain that this is the right approach. With the optimization, the buffer will be rewritten from start if it was empty after the previous dequeue_many_with(), hence the spec changes.

whitequark · 2018-04-12T01:53:15Z

It seems we have two separate things at work here:

A possible bug: "I've studied the code for a while and wondered why there is a check for capacity > 0 at this location. capacity doesn't change. I believe self.length was actually intended here."
An optimization (?): "In the RawSocket use by DHCPv4 client #186 that's needed otherwise PacketBuffer tends to get a stuck payload_ring (length=0 but read_at near capacity)."

@astro, can you please provide separate test cases for (1) and (2)? If the check is genuinely dead, then let's first remove it, and then separately examine the optimization (2); I don't fully understand why it is necessary yet.

astro · 2018-04-12T16:50:37Z

Ok, I reordered this PR with the updated tests first and had CI trigger on them.

Now I've put the optimization into enqueue_many_with() which is the method that benefits from this. Subsequently I had to fundamentally touch test_buffer_wraparound_tx() to trigger the intended check.

whitequark

The code change looks good to me but the change to the TCP test_buffer_wraparound_tx seems wrong.

whitequark · 2018-04-22T02:09:12Z

src/socket/tcp.rs

-        assert_eq!(s.send_slice(b"abc"), Ok(3));
+        s.tx_buffer = SocketBuffer::new(vec![b'.'; 9]);
+        assert_eq!(s.tx_buffer.enqueue_many(6).len(), 6);
+        assert_eq!(s.tx_buffer.dequeue_many(3).len(), 3);


Why are you manipulating the buffer directly in this test and not via the socket API? This defeats the purpose of testing the socket.

Because this is just preparing the buffer. Anyway, fixed it.

… instead of comparing the buffer. Fixtures have been shortened to work with the outstanding contiguous buffer optimization.

This is still testing the same case which would be masked away by the upcoming contiguous space optimization (which is important for PacketBuffers and still helpful to TcpSockets).

…ation.

whitequark · 2018-04-23T20:50:58Z

src/socket/tcp.rs

-        assert_eq!(s.send_slice(b"abc"), Ok(3));
+        s.tx_buffer = SocketBuffer::new(vec![b'.'; 9]);
+        assert_eq!(s.send_slice(b"xxxyyy"), Ok(6));
+        assert_eq!(s.tx_buffer.dequeue_many(3), &b"xxx"[..]);


This is not any better...

Why touch the internal tx_buffer you ask? The read_at pointer must be initialized so that the check still makes sense when the optimization is merged.

whitequark · 2018-05-24T22:00:10Z

@astro Thanks, and sorry for taking so much time to merge this.

astro mentioned this pull request Apr 9, 2018

DHCPv4 client #186

Closed

astro force-pushed the fix-ringbuffer branch from e675f27 to 4b1fc46 Compare April 9, 2018 21:58

dlrobertson reviewed Apr 10, 2018

View reviewed changes

astro force-pushed the fix-ringbuffer branch from 4b1fc46 to 1caef79 Compare April 12, 2018 16:44

astro force-pushed the fix-ringbuffer branch from 4297051 to 4a4a260 Compare April 12, 2018 16:51

astro changed the title ~~Make the RingBuffer contiguous memory optimization work.~~ RingBuffer contiguous memory optimization Apr 13, 2018

whitequark mentioned this pull request Apr 17, 2018

Support for receiving fragmented IPv4 packets #190

Closed

astro mentioned this pull request Apr 19, 2018

Resource exhaustion test #193

Closed

whitequark requested changes Apr 22, 2018

View reviewed changes

astro added 5 commits April 23, 2018 22:37

Rewrite test_buffer_write_unallocated() to use ring.get_unallocated()…

c29b400

… instead of comparing the buffer. Fixtures have been shortened to work with the outstanding contiguous buffer optimization.

Rewrite test_buffer_wraparound_tx() by manually prefilling the buffer.

69e1e7e

This is still testing the same case which would be masked away by the upcoming contiguous space optimization (which is important for PacketBuffers and still helpful to TcpSockets).

Add test_buffer_write_wholly() to check for contiguous memory optimiz…

07da8da

…ation.

Add RingBuffer.enqueue_many_with() optimization for contiguous memory.

3858742

Replace test_buffer_wraparound_tx enqueue_many() with send_slice()

8c4e2ec

astro force-pushed the fix-ringbuffer branch from 4297051 to 8c4e2ec Compare April 23, 2018 20:48

whitequark reviewed Apr 23, 2018

View reviewed changes

whitequark approved these changes May 24, 2018

View reviewed changes

Merge branch 'master' into fix-ringbuffer

6e2da5e

whitequark merged commit 908ef8d into smoltcp-rs:master May 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

RingBuffer contiguous memory optimization #187

RingBuffer contiguous memory optimization #187

astro commented Apr 9, 2018

Uh oh!

dlrobertson left a comment •

edited

Loading

Uh oh!

astro commented Apr 10, 2018

Uh oh!

whitequark commented Apr 10, 2018 via email

Uh oh!

astro commented Apr 10, 2018

Uh oh!

whitequark commented Apr 12, 2018

Uh oh!

astro commented Apr 12, 2018

Uh oh!

whitequark left a comment

Uh oh!

whitequark Apr 22, 2018

Uh oh!

astro Apr 23, 2018

Uh oh!

whitequark Apr 23, 2018

Uh oh!

astro Apr 23, 2018

Uh oh!

whitequark commented May 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

RingBuffer contiguous memory optimization #187

RingBuffer contiguous memory optimization #187

Conversation

astro commented Apr 9, 2018

Uh oh!

dlrobertson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astro commented Apr 10, 2018

Uh oh!

whitequark commented Apr 10, 2018 via email

Uh oh!

astro commented Apr 10, 2018

Uh oh!

whitequark commented Apr 12, 2018

Uh oh!

astro commented Apr 12, 2018

Uh oh!

whitequark left a comment

Choose a reason for hiding this comment

Uh oh!

whitequark Apr 22, 2018

Choose a reason for hiding this comment

Uh oh!

astro Apr 23, 2018

Choose a reason for hiding this comment

Uh oh!

whitequark Apr 23, 2018

Choose a reason for hiding this comment

Uh oh!

astro Apr 23, 2018

Choose a reason for hiding this comment

Uh oh!

whitequark commented May 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

dlrobertson left a comment •

edited

Loading