gcc build warning, switched to catch a reference#2
Open
bryancall wants to merge 1 commit into
Open
Conversation
|
LGTM |
nshopik
referenced
this pull request
in nshopik/pping2
May 5, 2026
Hot per-packet path was bottlenecked on std::string allocation: 5-6 string-keyed map lookups + ~15-25 short-lived std::string construct/ destroy/concatenate/heap calls per packet, dominated by the cost of turning libtins addresses into "src:port+dst:port" keys. This rewrite replaces the keys with packed POD structs whose raw bytes are hashed directly: - FlowKey: 40B (16B src + 16B dst + 2B sport + 2B dport + 1B af + 3B pad). v4 addresses live in the first 4 bytes of the 16B slot, with the trailing 12 bytes zero. The af field disambiguates v4 from v6 packets that happen to share the first 4 IP bytes. - TsKey: FlowKey + 4B tsval + 4B pad = 48B. - ByteHash: FNV-1a over sizeof(T) raw bytes. Padding participates, so default member initializers + an explicit _pad array are used to guarantee the pad bytes are zero on every key. static_asserts on sizeof guard the layout. The hot-path lookup count drops to: 1x flows.try_emplace(fk) forward flow 1x tsTbl.try_emplace(tk) store our TSval 1x tsTbl.find(rk+ecr) match against reverse flow's saved TSval 1x flows.find(rk) (only on RTT match) update reverse bytesDep 1x flows.find(rk) (only on new flow) bidirectional setup IP byte conversion (IPv4Address::to_string / IPv6Address::to_string) is deferred to the print path and only happens on actual RTT matches. tsTbl now stores tsInfo by value instead of tsInfo*. This folds the TODO #2 fix in (the duplicate-key insert path used to leak ti when try_emplace returned false; with by-value storage the leak is structurally impossible). cleanUp loses its delete on tsInfo. Other changes: - localIP gets a parallel localIPBytes/localIPaf representation parsed once at startup so the filtLocal check is a 16-byte memcmp instead of a std::string compare. - flowRec drops its flowname field (unused after the rewrite); the flow-limit-reached log message uses a flowKeyName(FlowKey) helper. - getTStm wrapper is gone — process_packet calls tsTbl.find directly and reads/mutates eit->second in place. - Bump CXXFLAGS -std=c++14 -> c++17. try_emplace and structured bindings (in the plan's pseudocode and used here) need it; the old __cpp_lib_unordered_map_try_emplace fallback is removed. Tests: - Migrated test_addTS and test_cleanUp to FlowKey/TsKey + by-value. - New test_flowkey_padding (the load-bearing one — verifies sizeof, pad bytes are zero, and two independently-built equal keys have identical bytes and ByteHash output). - New test_flowkey_reversed (round-trip and field swaps). - New test_flowkey_v4_v6_disambig (same first 4 IP bytes + different af must NOT compare or hash equal). - New test_addTS_no_leak_on_duplicate (asserts size/first-write-wins on the dup-key path that used to leak). The pcap integration test (test/known.pcap.golden) and the -m/-e format regression test are unchanged and still gate the output format. TODOS.md: drop the two now-resolved items (tsTbl bound was already done in 2a6fc72; tsInfo leak is fully resolved here). Benchmark on bench.pcap (232,718 packets, warm page cache, median of 6 runs): master (4b8c778): 3495 ns/pkt, 0.286 Mpps this commit (rewrite only): 2906 ns/pkt, 0.344 Mpps (1.20x) Below the plan's 3-5x target. perf record on the warm path showed libtins parsing + TCP option exception handling dominating the remaining cost. The exception path is addressed in a separate commit on this branch (search_option for the TCP timestamp option) which brings the median to ~1944 ns/pkt — 1.80x vs master overall. The larger libtins replacement remains a deferred PR per the plan's NOT-in-scope section. Plan: ~/.gstack/projects/pollere-pping/bill-master-eng-review-plan-20260505.md Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
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
2 participants
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.
Error message when building: