Improve route level parser#287
Conversation
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
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 yieldsRouteBatch(one batch per route-bearing record / UPDATE), withRouteBatch::routes()andRouteBatch::all_routes()producing per-prefix rows lazily.BgpRouteElembecomes a borrowed row (BgpRouteElem<'a>) borrowing&AsPath, withBgpRouteElemOwned+into_owned()for ownership.- Route-level MP_REACH/MP_UNREACH and standard NLRI handling is reworked to store raw
Bytesand 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.
| 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; | ||
| } |
| /// 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. |
| if afi == Afi::LinkState | ||
| || safi == Safi::LinkState | ||
| || safi == Safi::LinkStateVpn | ||
| || safi == Safi::MplsLabel | ||
| { | ||
| return Ok(None); | ||
| } |
|
I think this might be a performance regression. In an application, compared to baseline: |
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 yieldsRouteBatchvalues instead of directBgpRouteElemrows.RouteBatch::routes()for filtered route iteration andRouteBatch::all_routes()for unfiltered iteration.BgpRouteEleminto a borrowed route row,BgpRouteElem<'a>, withas_path: Option<&'a AsPath>.BgpRouteElemOwnedandBgpRouteElem::into_owned()for callers that need rows to outlive the containing batch.FallibleRouteIteratorto yieldResult<RouteBatch, ParserErrorWithBytes>.Bytesand parse prefixes lazily.Behavior Notes
This is a public API change for route-level parsing callers. Existing code like:
now needs to iterate through each batch:
Use batch.all_routes() when applying custom filters manually or when unfiltered batch contents are needed.