grant_max returns largest possible buffer

[sjroe]

Resolves #29

grant_max now looks to see if inverting allows the requested (or larger) buffer size to be returned.

[jamesmunns]

Hey @sjroe, thanks for the PR!

I'm inclined to merge this, would you mind expanding the comments for grant_max to clarify that this could cause an early wrap-around?

Also, I think it would be worth it to have functions that support both:

• Grant max, but don't wraparound if there is any space left (>= 1 byte)
• Grant max, always okay to wraparound to try and get all requested bytes

and maybe even

• Grant max, only try wraparound if there is less than N bytes available.

I'm open to naming suggestions here.

[jamesmunns]

Also CC @jonas-schievink for thoughts

[jonas-schievink]

Looks good, this is definitely the behavior most convenient for me.

[jonas-schievink]

This seems unnecessary since the grant was just created from the right buffer. Maybe replace it with

debug_assert!(self.is_our_grant(&grant.buf));?

[sjroe]

@jamesmunns I'm not averse to creating alternative grant functions but unsure on the naming.

Thanks @jonas-schievink - those 3 lines should have been removed; they are remenants of me tracking down an issue.

[jamesmunns]

@sjroe how about this (feel free to use the descriptions in the comments as well):

fn grant_max(sz: usize)

grant_max() attempts to find a contiguous region up to sz bytes large. If no such region exists, the largest region available will be granted to the user. This function may cause available bytes at the end of the bbqueue to be skipped, if a larger region (including up to sz bytes) is available at the beginning of the ring.

fn grant_max_remaining(sz: usize)

grant_max_remaining() will return the largest contiguous region up to sz bytes large without prematurely wrapping to the beginning of the ring. This is similar to grant_max(), however grant_max_remaining() will only ever wrap to the beginning of the ring when exactly zero bytes are available at the end of the ring.

Outside of these two options:

I'd like to see a test for what happens if you cause a wrap to happen with the new grant_max(), but commit no bytes. e.g:

• Start with a buffer of 128 items
• Grant + commit 65 bytes
• release 64 bytes
• grant_max(64) (this will be at the start)
• commit(0)

I think this will cause the 63 bytes at the end to be skipped, but shouldn't cause any long-term problems (once you release that 65th byte, the watermark will be moved to the end, freeing up those "dead" 63 bytes), though those 63 bytes will be unusable until the reader catches up to the watermark. I think this was the case I was trying to avoid with the original grant_max(). Ideally, you'd be able to detect this on a commit(0), but I'm not sure how to do this safely with respect to ordering.

[jamesmunns]

Also maybe some helpful ascii graphs (I can't remember if the Reader and Writer are allowed to equal eachother, these might need to be tweaked)

# Initial State

* 32 bytes total
* 7 available at the end
* 8 available at the beginning

 00000000001111111111222222222233
 01234567890123456789012345678901
.________________________________.
|                                |
'--------------------------------'
          R               W      M

# grant_max(8)

* 32 bytes total
* 7 dead at the end
* 8 granted for writing at beginning

 00000000001111111111222222222233
 01234567890123456789012345678901
.________________________________.
|gggggggg                 xxxxxxx|
'--------------------------------'
         WR               M      

# Initial state

* 32 bytes total
* 7 available at the end
* 8 available at the beginning

 00000000001111111111222222222233
 01234567890123456789012345678901
.________________________________.
|                                |
'--------------------------------'
          R               W      M

# grant_max_remaining

* 32 bytes total
* 7 granted for writing at end
* 8 available at the beginning

 00000000001111111111222222222233
 01234567890123456789012345678901
.________________________________.
|                         ggggggg|
'--------------------------------'
          R                      M
                                 W

[sjroe]

No problem, I'll look to put all of that together shortly.

I'm not keen on the description for grant_max_remaining; particularly the part without prematurely wrapping... as I don't feel that wrapping to provide the requested buffer size is wrapping prematurely. I propose that the word prematurely is removed as it doesn't affect the clariy. In the original article by Simon Cooke, he states:

Once more free space is available to the left of region A than to the right of it, a second region (comically named "region B") is created in that space. The choice to create a new region when more space is available on the left is made to maximize the potential free space available for use in the buffer.

Regarding the test case with 0 bytes comitted, grant_max will yield the same behaviour as grant. If a test case doesn't already exist for this scenario for grant then I'll generate but I belive that both of the randomised santiy check tests (sanity_check and sanity_check_grant_max) will check this scenario.

I liked your earlier suggestion of functions as per grant_max and grant_max_remaining with a second parameter providing the minimum acceptable buffer size. Do you not think it would be better to extend grant_max and grant_max_remaining rather than create an additional 2 functions?

[sjroe]

@jamesmunns I implemented the 0 bytes comitted test and it fails for both grant and grant_max:

let bb = bbq!(100).unwrap();

// Grant and commit more than half of the buffer
let wgr = bb.grant(60).unwrap();
bb.commit(60, wgr);

// And release all but one
let rgr = bb.read().unwrap();
assert_eq!(rgr.len(), 60);
bb.release(59, rgr);

// Request grant where there is enough room if invert
let wgr = bb.grant(55).unwrap();
assert_eq!(wgr.len(), 55);

// Commit 0 bytes
bb.commit(0, wgr);

// Release the remaining one byte from the original commit
let rgr = bb.read().unwrap();
assert_eq!(rgr.len(), 1);
bb.release(1, rgr);

// Should now be at position 0 of an empty 100 byte buffer
// Grant and commit to near the end
let wgr = bb.grant(99).unwrap(); // <- fails with Err value: InsufficientSize

If using grant_max rather than grant for the last line then wgr.len() == 59 which implies that the watermark hasn't moved to the end.

This means that everytime there is a commit < grant size where the grant causes an invert with dead bytes at the end the total usable buffer shrinks.

I will track down and resolve but this is a new issue; do you want a separate issue filed?

[jonas-schievink]

I tried using this branch, and it seems that this addition can overflow when passing usize::max(), which kinda makes sense, but wasn't happening before. This probably isn't a big issue, but still a possibly surprising side-effect.

[jamesmunns]

Hey @sjroe, I know it's been a while, but I'm working on the 0.4.0 release in #37, and I plan to support the following kinds of grants in the future (not all of these may make it into 0.4.0, but should be in 0.4.x releases). Let me know if you think this all makes sense:

As a short summary of possible grants:

• grant_max_remaining(N)
  • User will receive a grant 0 < sz <= N (or receive an error)
  • This will only cause a wrap to the beginning of the ring if exactly
    zero bytes are available at the end of the ring.
  • Maximum possible waste due to skipping: 0 bytes
• grant_remaining()
  • User will receive a grant 0 < sz <= total_buffer_sz (or receive an error)
  • This will only cause a wrap to the beginning of the ring if exactly
    zero bytes are available at the end of the ring.
  • Maximum possible waste due to skipping: 0 bytes
• grant_largest()
  • User will receive a grant 0 < sz <= total_buffer_sz (or receive an error)
  • This function will find the largest contiguous region available
    (at the end or beginning of the ring).
  • If the region at the beginning was chosen, some bytes at the end of the ring
    will be skipped
  • Maximum possible waste due to skipping: (total_buffer_sz / 2) - 1 bytes
• grant_largest_max(N)
  • User will receive a grant 0 < sz <= N (or receive an error)
  • This function will attempt to find a contiguous region up to sz bytes large.
    If no such region exists, the largest region available (at the end or
    beginning of the ring) will be granted to the user.
  • If the region at the beginning was chosen, some bytes at the end of the ring
    will be skipped
  • Maximum possible waste due to skipping: (total_buffer_sz / 2) - 1 bytes
• grant_exact(N)
  • User will receive a grant sz == N (or receive an error)
  • This may cause a wraparound if a grant of size N is not available
    at the end of the ring.
  • If this grant caused a wraparound, the bytes that were "skipped" at the
    end of the ring will not be available until the reader reaches them,
    regardless of whether the grant commited any data or not.
  • Maximum possible waste due to skipping: (total_buffer_sz / 2) - 1 bytes

The following might introduce the concept of "split grants", which provide two
separate contiguous buffers in order to eliminate waste due to splitting, but require
the user to make writes to each buffer.

• split_grant_remaining(N)
  • User will receive a grant containing two segments with a total size of
    0 < (sz_A + sz_B) <= total_buffer_sz (or receive an error)
• split_grant_max_remaining(N)
  • User will receive a grant containing two segments with a total size of
    0 < (sz_A + sz_B) <= N (or receive an error)
  • If the grant requested fits without wraparound, then the sizes of the grants
    will be: sz_A == N, sz_B == 0.
• split_grant_exact(N)
  • User will receive a grant containing two segments with a total size of
    (sz_A + sz_B) == N (or receive an error)
  • If the grant requested fits without wraparound, then the sizes of the grants
    will be: sz_A == N, sz_B == 0.

[jamesmunns]

Please see #38 for additional work on this topic.