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

Performance Improvements #125

Draft
wants to merge 26 commits into
base: main
Choose a base branch
from
Draft

Conversation

jmeggitt
Copy link
Contributor

@jmeggitt jmeggitt commented Sep 12, 2023

The aim of this pull request is to improve overall parsing performance. These changes have largely been motivated by profiles conducted using Intel VTune, statistics on the general arrangement of MRT records collected using https://github.com/jmeggitt/bgp_attribute_survey, the new mrt_type benchmark, and personal intuition regarding performance.

Most of these changes are related to allocation and #85 .

Despite being quite lengthy, this pull request is not complete as there are still a couple of areas I want to get to. Particularly, I would like to make an AttributesStorage type which allows inlining some of the most common attribute groups. I will update this description with additional details on what/why various things are being done when this draft is converted to a regular pull request.

Currently, these changes improve performance by roughly ~2x as shown by the mrt_type benchmark.

@jmeggitt
Copy link
Contributor Author

Blocked by #123 and #124 .

@jmeggitt
Copy link
Contributor Author

Due to the size of these changes, it may be better to split the AsPath storage into its own pull request.

In fact, considering the amount of code added to AsPath across the most recent pull requests, it may even be worth considering turning it into its own crate.

@digizeph
Copy link
Member

Due to the size of these changes, it may be better to split the AsPath storage into its own pull request.

In fact, considering the amount of code added to AsPath across the most recent pull requests, it may even be worth considering turning it into its own crate.

I'm fine with this PR size. Not sure about separating it into its own crate though. Users can use this crate without default parser feature to get only the bgp module, which should be sufficient.

@digizeph
Copy link
Member

digizeph commented Sep 17, 2023

EDIT: Added back in PR #127

Can we add back the previously available .to_u32_vec function? Or you've added something equivalent to quickly get a Vec from a given AS path (or None)?

Something like this:

    pub fn to_u32_vec(&self) -> Option<Vec<u32>> {
        match self.segments.last() {
            Some(AsPathSegment::AsSequence(v)) => Some(v.iter().map(|asn| (*asn).into()).collect()),
            _ => None,
        }
    }

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

Successfully merging this pull request may close these issues.

2 participants