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

Change trait method signature of octet string decoding to use generic #374

Merged
merged 6 commits into from
Nov 25, 2024

Conversation

Nicceboy
Copy link
Contributor

For UPER, performance boosts is around 20x
For OER, around 2,5x

BER and JER do not use constraints for this case.

image

@XAMPPRocky
Copy link
Collaborator

Thank you for your PR! However I think in regards to the trait method, I think it would be better to move the octet_string to having a generic for its return type. The motivation for that being, that it would reduce API surface, and allow us to support multiple different container types (e.g. smallvec).

src/ber/de.rs Outdated
Comment on lines 462 to 466
// BER is not aware of the constraints
let data = self.decode_octet_string(tag, constraints)?;
let mut array = [0u8; N];
array.copy_from_slice(data.as_slice());
Ok(array)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BER is not constraint aware, meaning that it doesn't make any format optimisations, but we can still optimise and check this case. If we're parsing a fixed length octet we can still both check that the length matches the fixed length constraint, and copy directly from the input into the array, skipping the vec here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point!

@Nicceboy
Copy link
Contributor Author

The branching overhead was quite measurable in this case. Having own special function optimizes into single memcpy.

I can try the generic version again if it can be avoided.

@Nicceboy Nicceboy force-pushed the fixed-size-octed-decoding branch 2 times, most recently from 7da7194 to 1b09605 Compare November 25, 2024 07:17
@Nicceboy Nicceboy force-pushed the fixed-size-octed-decoding branch 2 times, most recently from 75aab27 to bf35b2f Compare November 25, 2024 07:18
@Nicceboy
Copy link
Contributor Author

I changed the method to have the following signature:

/// Decode a `OCTET STRING` identified by `tag` from the available input.
fn decode_octet_string<'buf, T>(
    &'buf mut self,
    tag: Tag,
    constraints: Constraints,
) -> Result<T, Self::Error>
where
    T: From<&'buf [u8]> + From<Vec<u8>>;

We must support both owned (mainly because of JER) and sliced types.

OER can take the most benefits from this already. It also get some improvements for internal usage of parsing octet strings because it is possible to pass references with Cow<[u8]> type.

However, I think BER and PER optimization should be done separately. BER gets partial improvement for decoding primitive variants as it has access to the slice and can return it.

For other cases, the function need a major redesign so that slices can be passed around.

PER optimization should be done at the same time as the overall code structure is reviewed to be more friendly for borrows and reusing buffers. Current closure-based approach makes it harder to work with borrows.

I think that major part for PER's slowness compared to other codecs is the parsing with nom_bitvec and also other use of BitVec.

I made only small change now for decoding_octet_string (different way to convert BSlice to vector) and that already doubled the decoding speed for octet strings.

image

Since extensions are encoded as open type in OER, (includes decoding with octet strings), internal slice references improved the integer bench as well:

image

@Nicceboy Nicceboy changed the title Add own trait method for fixed-size octet string decoding Change trait method signature of octet string decoding to use generic Nov 25, 2024
@Nicceboy
Copy link
Contributor Author

Main branch might need a manual fix. Something related to partial release of the packages?

@XAMPPRocky XAMPPRocky merged commit 2ea35d0 into librasn:main Nov 25, 2024
65 checks passed
@XAMPPRocky
Copy link
Collaborator

XAMPPRocky commented Nov 25, 2024

The branching overhead was quite measurable in this case.

We must support both owned (mainly because of JER) and sliced types.

For other cases, the function need a major redesign so that slices can be passed around.

Yeah I'm not sure what the final API looks like, but I think we can combine the constraint validation with the path we should take for the container. E.g. If we have a fixed size constraint, we can try to directly copy into the container. If we don't have a fixed size constraint, but a fixed size container we return an error.

trait SequenceOf<T> {
    const SIZE: Option<Size> = None;
}

enum Size {
    Fixed(usize),
    Minimum(usize),
    Maximum(usize),
    Range(usize, usize),
}

@Nicceboy Nicceboy deleted the fixed-size-octed-decoding branch November 25, 2024 11:39
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.

2 participants