Page MenuHomePhabricator

[services][backup] PullBackup 3/5 - response buffer utility
ClosedPublic

Authored by bartek on Jan 11 2023, 3:23 PM.
Tags
None
Referenced Files
F3492112: D6242.id21096.diff
Wed, Dec 18, 10:15 PM
F3491740: D6242.diff
Wed, Dec 18, 8:12 PM
Unknown Object (File)
Thu, Nov 28, 4:29 AM
Unknown Object (File)
Thu, Nov 28, 4:29 AM
Unknown Object (File)
Thu, Nov 28, 4:29 AM
Unknown Object (File)
Thu, Nov 28, 4:29 AM
Unknown Object (File)
Thu, Nov 28, 4:29 AM
Unknown Object (File)
Thu, Nov 28, 4:29 AM
Subscribers

Details

Summary

First part of my attempt to separate transport and business logic, because it is terribly mixed in the C++ codebase.

This diff implements the ResponseBuffer utility struct which handles retrieving data chunks of arbitrary size. One of its functionalities is to reflect this C++ function.

Depends on D6241

Test Plan

This structure is used two diffls later and tested altogether with the whole endpoint. I'm wondering if it's worth adding unit tests here.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Jan 11 2023, 3:55 PM
tomek requested changes to this revision.Jan 12 2023, 8:47 AM

I'm really glad to see this logic written in a lot cleaner way!

services/backup/src/service/handlers/pull_backup.rs
143

What's the purpose of the padding?

146

I worry about the performance here. According to the docs https://doc.rust-lang.org/std/vec/struct.Vec.html#method.split_off

Returns a newly allocated vector containing the elements in the range [at, len)

which means that if we have to make k splits and the original vector had size n we will have O(nk) memory cells allocated (the first split allocates a new vector of size n - k, 2nd - n - 2k, etc. which sums up to O(nk).)

Can we find a solution that doesn't have this issue?

This revision now requires changes to proceed.Jan 12 2023, 8:47 AM
services/backup/src/service/handlers/pull_backup.rs
143

Padding is the number of bytes to shrink the destination chunk.
Example:
We have limit = 100bytes and padding = 5bytes
Then the returned chunk size will be limit - padding = 95bytes

A more obvious version of this function:

/// Gets chunk of size `limit - padding` and leaves remainder in buffer
pub fn get(&mut self, padding: usize) -> Vec<u8> {
  let mut chunk = std::mem::take(&mut self.buf);

  let target_size = self.limit - padding;
  if chunk.len() > target_size {
    // after this operation, chunk=0..target_size, self.buf=target_size..end
    self.buf = chunk.split_off(target_size);
  }
  return chunk;
}

I noticed that my original one had a mistake

146

Yeah, this allocation is a known problem (example).

This could be done with some unsafe pointer logic, but we're using Rust for a reason ;)

I think the problem is elsewhere. The returned chunk must be "moved out" to the response stream, so this memory should be considered "lost", we have no more control over it. Without reallocations, we lose a part of our buffer memory each time a chunk is retrieved. So if we have k chunks, we must perform k allocations for new chunks, or one huge allocation for all chunks (unknown number anyway).

And I think that we should care more about number of allocations/data copies rather than a total number of memory cells allocated, as data in the memory gets rotated, it doesn't grow up statically (sent responses are finally deallocated) so we won't end up with O(nk) allocated memory at once.

In this case, we're doing two-three memory operations per chunk:

  1. extending buffer with new data (0-1 realloc, 1-2 copies, depending on prev capacity)
  2. "moving out" chunk with std::mem::take - it allocs an empty Vec (1 alloc)
  3. split off - (1 alloc, 1 copy) - see code (lines 2119-2128)

At this moment I cannot think of a better solution for this

services/backup/src/service/handlers/pull_backup.rs
146

If you're worried about memory size allocated for the buffer, I made some approximations.
Let's assume we're receiving 4MB chunks (gRPC limit) and have 1kB padding (likely real-world size). In this case, the buffer grows by 1kB for each chunk. It will reach 4MB (input chunk size) after 3999 chunks (nearly 16GB of data processed)

  • Renamed the get function to get_chunk and slightly rewrote its code to make it more obvious.
  • Added a test demonstrating working principle of the buffer.

After the discussion, I decided to left the buffer's working principle unchanged.

tomek added inline comments.
services/backup/src/service/handlers/pull_backup.rs
146

Ok, that makes sense. In general case it would be inefficient (e.g. if we created a vec of 1GB and then read from it in 1kB chunks) but for our use case it should work ok.

This revision is now accepted and ready to land.Jan 16 2023, 8:13 AM