Skip to content

Improve route level parser#287

Closed
ties wants to merge 3 commits into
bgpkit:mainfrom
ties:route-batches-efficiency
Closed

Improve route level parser#287
ties wants to merge 3 commits into
bgpkit:mainfrom
ties:route-batches-efficiency

Conversation

@ties

@ties ties commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR improves the route-level parser by changing route iteration from eager per-prefix rows to route batches with shared parsed state. The new batching model avoids repeatedly cloning shared data such as AS paths and delays NLRI prefix parsing until route rows are actually consumed.

A routebatch maps cleanly on one BGP message. Main question is if it improves performance, current API is more ergonomic.

What Changed

  • into_route_iter() now yields RouteBatch values instead of direct BgpRouteElem rows.
  • Added RouteBatch::routes() for filtered route iteration and RouteBatch::all_routes() for unfiltered iteration.
  • Changed BgpRouteElem into a borrowed route row, BgpRouteElem<'a>, with as_path: Option<&'a AsPath>.
  • Added BgpRouteElemOwned and BgpRouteElem::into_owned() for callers that need rows to outlive the containing batch.
  • Updated FallibleRouteIterator to yield Result<RouteBatch, ParserErrorWithBytes>.
  • Reworked route-level MP_REACH/MP_UNREACH and standard NLRI handling to store raw Bytes and parse prefixes lazily.
  • Preserved AS path sharing across all announced prefixes in the same route batch.
  • Updated route filtering to work against borrowed route rows inside each batch.
  • Updated the route-level parsing example to consume batches and iterate routes explicitly.

Behavior Notes

This is a public API change for route-level parsing callers. Existing code like:

for route in parser.into_route_iter() {
    // use route
}

now needs to iterate through each batch:

for batch in parser.into_route_iter() {
    for route in batch.routes() {
        let route = route?;
        // use route
    }
}

Use batch.all_routes() when applying custom filters manually or when unfiltered batch contents are needed.

@codecov

codecov Bot commented Jun 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.27354% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.03%. Comparing base (d9889d1) to head (4ba8326).

Files with missing lines Patch % Lines
src/parser/iters/route.rs 92.85% 30 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #287      +/-   ##
==========================================
+ Coverage   89.69%   90.03%   +0.33%     
==========================================
  Files          86       86              
  Lines       18334    18657     +323     
==========================================
+ Hits        16444    16797     +353     
+ Misses       1890     1860      -30     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the route-level parsing API to iterate by BGP message/record “batches” with shared parsed state, enabling lazy NLRI prefix parsing and reducing repeated cloning (notably AS paths) across per-prefix route rows.

Changes:

  • into_route_iter() now yields RouteBatch (one batch per route-bearing record / UPDATE), with RouteBatch::routes() and RouteBatch::all_routes() producing per-prefix rows lazily.
  • BgpRouteElem becomes a borrowed row (BgpRouteElem<'a>) borrowing &AsPath, with BgpRouteElemOwned + into_owned() for ownership.
  • Route-level MP_REACH/MP_UNREACH and standard NLRI handling is reworked to store raw Bytes and parse prefixes on-demand; filter plumbing updated accordingly.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/parser/utils.rs Exposes try_parse_prefix within the crate to support lazy NLRI parsing.
src/parser/iters/route.rs Implements RouteBatch + lazy NLRI parsing/iteration and updates route-level iterators to yield batches.
src/parser/iters/mod.rs Re-exports new batch types and updates route-iterator docs to match the new API.
src/parser/filter.rs Updates filtering traits/implementations to work with borrowed BgpRouteElem<'_>.
src/models/bgp/elem.rs Converts BgpRouteElem into a borrowed type and introduces BgpRouteElemOwned.
examples/route_level_parsing.rs Updates the example to consume batches and then iterate routes within each batch.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/parser/iters/route.rs
Comment on lines +240 to +245
let data = self.input.as_ref();
if !self.is_add_path && !data.is_empty() && data[0] == 0 {
debug!("NLRI: first byte is 0, treating as Add-Path format");
self.is_add_path = true;
self.use_heuristic = true;
}
Comment thread src/models/bgp/elem.rs
Comment on lines 163 to 165
/// peer metadata, timestamp, and AS path. Use [`BgpElem`] when you need the
/// full set of BGP attributes. Because route elements do not carry
/// communities, community filters do not match [`BgpRouteElem`] values.
Comment thread src/parser/iters/route.rs Outdated
Comment on lines +79 to +85
if afi == Afi::LinkState
|| safi == Safi::LinkState
|| safi == Safi::LinkStateVpn
|| safi == Safi::MplsLabel
{
return Ok(None);
}
@ties

ties commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

I think this might be a performance regression. In an application, compared to baseline:

ingest_mrt/load_mrt_file/rrc13_bview_20260403_0000
                        time:   [2.4200 s 2.4230 s 2.4264 s]
                        change: [+24.610% +24.911% +25.241%] (p = 0.00 < 0.05)
                        Performance has regressed.

@ties ties closed this Jun 8, 2026
@ties ties deleted the route-batches-efficiency branch June 8, 2026 19:30
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