Repository navigation
Conversation
The benchmark targets could not run on a clean checkout. The test data download opened the target with get_reader (which decompresses by suffix) before File::create truncated it, so the copy always ended in "decompression not finished but EOF reached"; the checksum was also computed over decompressed bytes. Both targets resolved the target directory by walking up for a literal target/, which breaks under CARGO_TARGET_DIR or build.target-dir, and bench_main panicked when the CLI binary was missing even though cargo bench never builds it (required-features = ["cli"]). - Download to a .part file, hash the raw bytes, then rename. - Resolve the target directory from CARGO_TARGET_TMPDIR, keeping the walk-up as a fallback for older toolchains. - Skip bgpkit-parser in bench_main with a hint instead of panicking. - Benchmark the whole in-memory corpus instead of the first 100k items, which meant different record counts per mode, and report throughput (records/s or elems/s) next to the wall time. - Give the record-to-elem benchmark the peer index table it needs; RIB records were silently returning no elems without it. - Add opt-in real archives via BGPKIT_BENCH_RIB/BGPKIT_BENCH_UPDATES, streamed from disk so decompression is included. - Rewrite benches/README.md around the actual commands and variables.
- parse_as_path keeps segments inline and decodes ASNs straight into the segment SmallVec instead of growing a Vec and copying it in. - parse_attributes sizes its attribute vector from the section length with a realistic per-attribute floor instead of one slot per three bytes, which over-allocated by an order of magnitude on short records. - parse_rib_afi_entries reserves one slot per entry instead of two. Measured on a fixed workload with perf stat, which is stable to 0.01% across runs on this host: updates elems -2.0%, updates records -2.9%, RIB elems -1.7%, RIB records -2.0%. Allocations per RIB record 14.0 -> 11.9.
Framing a record allocated a Vec for the captured header bytes and a Bytes for the header itself, both only needed by the raw-record API and by error reporting. Read the header into a fixed 16-byte buffer, keep the body Vec, and build the header Bytes only where the raw API asks for it. Truncated headers still keep the bytes they did contain, so the existing partial-framing error tests pass unchanged. Measured with perf stat on a fixed workload (instruction counts are stable to 0.01% across runs on this host): updates records -7.1%, updates elems -3.5%, RIB records -3.2%, RIB elems -2.6% against the previous commit.
AS_PATH is in essentially every announcement, and it was the last attribute that had to be split out of the record body into its own Bytes before parsing. Parse it straight from a borrowed slice instead. Other attributes keep the owned path: routing all thirteen parses through one Option<Result<..>> dispatcher measured 6.8% slower on RIB records, so the win depends on the dispatch staying in the parse loop. Measured with perf stat on a fixed workload: RIB elems -2.6%, RIB records -3.1%, updates elems -2.4%, updates records -3.2% against the previous commit.
Adds benches/RESULTS.md with the before/after instruction counts, allocation counts, the harness defects that were fixed, the change that regressed and was reverted, and the reproduction commands. Un-ignores benches/*.md so benchmark documentation can live next to the benchmarks.
The stack-header rewrite advanced the captured length only after the read returned successfully, so an I/O error in the middle of the 4-byte microsecond field dropped the one to three bytes already read from the reported error bytes. An external review caught it; a clean end of stream was unaffected, which is why the existing 0/5/17-byte truncation tests stayed green. Read through a helper that advances the captured length in place, and cover 13, 14 and 15 header bytes for both an I/O error and a clean EOF.
A floor of four left most RIB entries growing their attribute vector: across all 470,642 fixture entries, 71.4% exceeded four slots and paid the realloc and copy. Six leaves 3.7% short, so eight was chosen; four, six and eight were measured against the same binary and stayed within 0.5% of each other. Instruction counts for the committed build against four slots, at a fixed iteration count: RIB elem iteration -3.2%, RIB record iteration -1.8%, updates elem iteration -2.9%, updates record iteration -0.5%. Allocations per RIB record 8.94 -> 7.24, per updates record 6.03 -> 5.55.
An external review of the harness found four ways a reader could be misled: - into_update_iter reported records/s while yielding update items (about 3.6% off on the updates fixture); - preload truncated at 64 MiB silently, which can cut a record in half and be hidden by the skipping iterators; - the record-to-elem and real-archive benchmarks divided by a fabricated 1 when they produced nothing; - the elementor benchmark still caps at 100k records, which RESULTS.md described as parsing the whole fixture. Fail loudly instead, and commit examples/perf_workload.rs so the fixed-workload instruction counts in RESULTS.md can actually be reproduced.
Updates RESULTS.md with the figures after the extended-timestamp fix and the attribute-capacity floor, states what the measurements do not cover, and adds a same-file comparison against CAIDA BGPStream with the probe and container used for it. BGPStream walks a full rrc00 bview with 116 G instructions and 9.1 s where this parser needs 392 G and 42.6 s; part of that gap is BGPStream handing out elems as views while BgpElem owns its attributes.
The bgpkit cell had been measured through the default output path (which formats and writes every element), not the -e counting path the rest of the table uses. On the counting path the fixture is 6.49 G instructions and 0.85 s, so the gap there is 1.7x rather than 3.3x; the full-feed rrc00 numbers are unaffected and re-verified at 392.4 G / 43.6 s.
…ord walk 0.09 s for 150360 records, so every cell in the comparison table now has both instructions and wall time.
The code and the headline measurements use eight slots, not six; the commit message and this record said six because an eight-slot experiment was left applied. All four/six/eight numbers are in the entry, and the end-to-end table was measured with eight.
Withdrawals and End-of-RIB messages carry no path attributes, yet every one of them reserved eight Attribute slots (1.5 KiB) before discovering the section was empty. Reserve nothing in that case. Measured on the real RIS updates file (150,360 records): elem iteration -0.25%, record iteration -0.36%. On the RIB fixture it costs 0.07%, where no record has an empty section.
These four carry a fixed-width value that is read once, so the per-attribute `Bytes` split only bought refcount traffic. Each now decodes straight from the attribute section, with its own branch: routing all four through one `Option<Result<..>>` dispatcher measured slower than the split it removes. Instruction counts against the previous commit at a fixed iteration count: RIB record iteration -1.1%, RIB elem iteration -0.9%, updates record iteration -0.7%, updates elem iteration -0.5%, real RIS updates record iteration -0.2%.
Adds the two kept changes, the Criterion medians against the pre-opt baseline, the end-to-end re-measurement, and why the dense peer lookup is deferred rather than dropped.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The probe treats BGPStream iteration errors as normal exhaustion and exits successfully, allowing failed runs to report partial counts as completed benchmarks and appear faster.
1 open issue
What changed in this PR
This PR repairs the benchmark harness and reduces allocations in MRT framing and BGP attribute parsing, with documented measurements showing roughly 16% fewer end-to-end parse instructions.
Changes:
- Fix fixture downloads and benchmark setup, report mode-specific throughput, and add opt-in real-archive runs.
- Keep MRT headers on the stack, decode common attributes from borrowed slices, and reduce vector allocations.
- Add a fixed-workload example, BGPStream comparison tools, and performance results.
| File | Description |
|---|---|
src/parser/utils.rs |
Enable parsing utilities for borrowed byte slices. |
src/parser/mrt/mrt_record.rs |
Use stack-backed headers and test partial-header error capture. |
src/parser/mrt/mrt_header.rs |
Add fixed-buffer framing with incremental byte capture. |
src/parser/mrt/messages/table_dump_v2/rib_afi_entries.rs |
Reduce RIB entry preallocation. |
src/parser/bgp/attributes/mod.rs |
Use slice-based fast paths and smaller attribute reservations. |
src/parser/bgp/attributes/attr_05_local_pref.rs |
Decode LOCAL_PREF from a slice. |
src/parser/bgp/attributes/attr_04_med.rs |
Decode MED from a slice. |
src/parser/bgp/attributes/attr_03_next_hop.rs |
Decode NEXT_HOP from a slice. |
src/parser/bgp/attributes/attr_02_17_as_path.rs |
Decode AS paths directly into segment storage. |
src/parser/bgp/attributes/attr_01_origin.rs |
Decode ORIGIN from a slice. |
examples/perf_workload.rs |
Add repeatable in-memory parsing workloads. |
benches/RESULTS.md |
Document measurements, methodology, and limitations. |
benches/README.md |
Explain benchmark setup and comparisons. |
benches/internals.rs |
Benchmark whole fixtures with mode-specific throughput. |
benches/data_source.rs |
Repair downloads and target-directory lookup. |
benches/bgpstream-compare/Dockerfile |
Provide the BGPStream comparison environment. |
benches/bgpstream-compare/bgps_probe.c |
Add a record and element counting probe. |
benches/bench_main.rs |
Skip the CLI benchmark when its binary is absent. |
.gitignore |
Allow benchmark Markdown documentation. |
Review effort: Balanced
Comment on lines
+44
to
+52
| int rv; | ||
| while ((rv = bgpstream_get_next_record(bs, &record)) > 0) { | ||
| records++; | ||
| if (elems_mode) { | ||
| while (bgpstream_record_get_next_elem(record, &elem) > 0) { | ||
| elems++; | ||
| } | ||
| } | ||
| } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #359 +/- ##
==========================================
+ Coverage 92.02% 92.06% +0.03%
==========================================
Files 97 97
Lines 24759 24916 +157
==========================================
+ Hits 22785 22939 +154
- Misses 1974 1977 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The benchmarks could not run on a clean checkout, so this branch starts with
benches/repaired: the fixture download truncated its own target (it opened the destination for decompression before creating it), the target-directory lookup panicked underCARGO_TARGET_DIR, and the CLI benchmark panicked when theclifeature was off. The suites now parse whole corpora instead of the first 100k records, report records and elems as separate denominators, and keep the real-corpus runs opt-in through environment variables.On that harness, six measured changes:
Vecplus aBytesper recordEnd to end on the rrc00 bview of 2026-10-14 (420 MB gz, 55.8 M elems,
-ecounting path, four alternating runs): 463.1 G to 390.6 G instructions (-15.7%) and 54.4 s to 42.2 s wall. Parse-only Criterion medians: RIB record iteration -18.9%, RIB elem iteration -11.4%, updates record iteration -17.2%.benches/RESULTS.mdcarries the method, the per-benchmark tables, the comparison against CAIDA BGPStream on the same file (3.4x instructions, 3.4x wall), and the change that was measured and reverted: routing all thirteen attribute parsers through one slice dispatcher cost 7% instead of saving it.Draft because the structural direction is not here: emitting elems from the record body instead of building the owned graph first is a separate design, and the dense peer lookup is deferred for now.