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

capnp: adding capnp::serialize::NoAllocSliceSegments and capnp::serialize::read_message_from_flat_slice_no_alloc #276

Merged
merged 1 commit into from
Jul 10, 2022

Conversation

tomkris
Copy link

@tomkris tomkris commented Jul 7, 2022

I'm adding an implementation for parsing unpacked capnp message which does not require heap allocations on happy path (only for error message if error occurs).

Context

no alloc support - #221

Currently all existing options for parsing capnp message require heap allocations.

For example, even SliceSegments will do heap allocation for storing segment offsets inside the Vec:

pub struct SliceSegments<'a> {
    words: &'a [u8],
    segment_indices : Vec<(usize, usize)>,
}

This is a problem for embedded applications where memory is scarce and heap allocations are either impossible or strongly discouraged, and having ability to parse message without doing heap allocations will improve experience of embedded developers using capnproto-rust.

In proposed implementation alloc crate is still required due to use of String in capnp::Error, however there will be no allocation happening on happy path, only in case of errors.

Implementation details

The header of the capnp message has only list of segment sizes. Offsets of segments can be calculated by combining segment sizes which are coming before segment of interest. Because of this, noalloc implementation has to parse at least first N segment info in the header every time NoAllocSliceSegments::get_segment is called. This is a performance downside of using noalloc implementaiton.

To partially mitigate the performance issue, I implemented a special case for messages with a single segment - they go through a separate enum variant and won't require message parsing on NoAllocSliceSegments::get_segment.

Proposed implementation works only for messages which are serialized in "non-packed" format. Packed messages would need to be unpacked first (which requires heap allocation) and it's something that need to be addressed additionally.

Considerations regarding reuse of read_segment_table

I attempted to reuse read_segment_table method in my implementation. Unfortunately it has heap allocation inside it:

let mut segment_sizes = vec![0u8; (segment_count & !1) * 4];

Resulting SegmentLengthsBuilder has Vec inside it:

pub struct SegmentLengthsBuilder {
    segment_indices: Vec<(usize, usize)>,
    total_words: usize,
}

it also parses all segments, when in my case it's desired to only parse first N segments.

I tried to make logic work for alloc and noalloc cases through injected traits, but it become too cumbersome, I realized that benefit of reusing that method outweighs increased complexity of the logic, so I decided to implement all logic from scratch.

Random number generation

I wanted to have a random number generator for generating test inputs. I introduced very naive random number generator, I was not sure if it's desired to have a build time dependency on rand crate, but I'll be happy to make a change and take the dependency on it if it's your preference.

…lize::read_message_from_flat_slice_no_alloc
@dwrensha
Copy link
Member

Thanks! This is really cool. Did you do any performance measurement of the SingleSegment special case? In the past, when I've experimented with smallvec, I've found that sometimes the added branching overhead outweighs any benefits.

@dwrensha
Copy link
Member

I wanted to have a random number generator for generating test inputs. I introduced very naive random number generator, I was not sure if it's desired to have a build time dependency on rand crate, but I'll be happy to make a change and take the dependency on it if it's your preference.

We already have a dependency on quickcheck, which depends on rand, so adding a new dev dependency on rand shouldn't actually pull in anything new. Can you make your tests just use quickcheck? I think that would be the simplest.

@dwrensha dwrensha merged commit 715ae0b into capnproto:master Jul 10, 2022
@dwrensha
Copy link
Member

I submitted a pull request to use the new function in serdebench, where I see a 20 to 25 percent improvement: llogiq/serdebench#18

@tomkris
Copy link
Author

tomkris commented Jul 12, 2022

We already have a dependency on quickcheck, which depends on rand, so adding a new dev dependency on rand shouldn't actually pull in anything new. Can you make your tests just use quickcheck? I think that would be the simplest.

Thanks, I'll make the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants