perf(hgraph): add reusable max_degree graph reduction - #2579
Conversation
|
/label status/waiting-for-review |
|
Automated pull request review failed. Review effort: the pull request head changed after /review; run the command again for the latest commit No GitHub review was submitted. |
Merge Protections🟢 All 2 merge protections satisfied — ready to merge. Show 2 satisfied protections🟢 Require kind label
🟢 Require version label
|
Signed-off-by: jc543239 <jc543239@antgroup.com> Assisted-by: Codex:gpt-5
59ac436 to
9df7758
Compare
| } | ||
| } | ||
|
|
||
| GraphInterfacePtr |
There was a problem hiding this comment.
[suggestion] The parallel_for helper has a subtle exception-safety issue: when GeneralEnqueue throws, the loop breaks, leaving already-submitted futures running. Those futures may also throw, and the collection loop silently discards later exceptions when first_exception is already set. More importantly, the remaining work items are never submitted, leaving the graph in a partially-processed state with no rollback.
Consider either:
- Collecting all exceptions and reporting them together, or
- Ensuring that partial execution is safe (e.g., by documenting that
rank_graphandmaterialize_graphare idempotent per-node and partial execution is acceptable).
At minimum, the break on submission failure should be replaced with a strategy that either cancels already-submitted work or documents why partial execution is safe.
|
|
||
| std::vector<std::pair<float, InnerIdType>> remaining; | ||
| remaining.reserve(original.size() - neighbors.size()); | ||
| for (const auto neighbor : original) { |
There was a problem hiding this comment.
[suggestion] The std::find on line 137 performs an O(N) linear scan over neighbors for each original neighbor, making the per-node complexity O(M*K) where M is the original degree and K is the number of selected neighbors. For graphs with large max_degree (e.g., 64 or 128), this becomes a noticeable hotspot.
Since neighbors is the result of select_edges_by_heuristic (typically small), consider using an std::unordered_set<InnerIdType> or sorting both vectors and using std::set_difference to avoid the quadratic behavior. Alternatively, since neighbors is modified in-place by select_edges_by_heuristic and then reversed, you could track which elements were kept during the heuristic selection itself rather than re-discovering them with std::find.
| materialize_graph(const GraphInterfacePtr& source, | ||
| const GraphInterfaceParamPtr& target_param, | ||
| const FlattenInterfacePtr& flatten, | ||
| Allocator* allocator, |
There was a problem hiding this comment.
[note] The flatten parameter in materialize_graph is only used for ExportCommonParam() to create the target graph instance. The actual neighbor copying does not use flatten at all — it only reads neighbor IDs from the source graph and writes them to the target. This is fine functionally, but the parameter name and presence may mislead readers into thinking distance computation is involved.
Consider either:
- Passing
CommonParamdirectly instead of the fullFlattenInterfacePtr, or - Adding a brief comment clarifying that
flattenis only used for graph instantiation, not for distance calculations.
| float build_cache_hit_rate_{-1.0F}; // cache hit rate from last cache-based build | ||
| uint64_t build_cache_hit_nodes_{0}; // number of nodes with cache hit | ||
| uint64_t build_cache_missed_nodes_{0}; // number of nodes without cache hit | ||
|
|
There was a problem hiding this comment.
[note] degree_reduction_prepared_ is not serialized/deserialized. After deserializing a previously-reduced graph, the flag will be false, so PrepareDegreeReduction would need to be called again before further reductions — which would re-rank the already-materialized neighbors unnecessarily. This is not incorrect but is wasteful.
If the reduced graph is intended to be further reducible after deserialization, consider either:
- Persisting the flag in the serialization format, or
- Making
PrepareDegreeReductiondetect that neighbors are already ranked (e.g., by checking if the first neighbor of each node is the closest).
LHT129
left a comment
There was a problem hiding this comment.
Overall this is a well-structured PR that introduces a useful internal capability for HGraph. The design is clean: rank adjacency lists once with the diversity heuristic, then materialize prefixes into compact storage for each target max_degree. The separation of PrepareDegreeReduction (rank) from ReduceMaxDegree (materialize) is a good choice that allows multiple reductions from a single ranking pass.
Summary of findings:
-
[suggestion]
parallel_forexception safety — Thebreakon submission failure leaves already-submitted futures running with no cancellation, and remaining work items are silently dropped. This leaves the graph in a partially-processed state. Consider documenting why this is safe or adding a rollback mechanism. -
[suggestion]
std::findinrank_graph— The O(M*K) linear scan per node can become a hotspot for largemax_degreevalues. Anunordered_setor set-based approach would be more efficient. -
[note]
materialize_graphflatten parameter — The parameter is only used forExportCommonParam()and is otherwise dead. Consider passingCommonParamdirectly or adding a clarifying comment. -
[note]
degree_reduction_prepared_not serialized — After deserialization the flag resets tofalse, causing unnecessary re-ranking if further reductions are attempted. This is not incorrect but is wasteful.
The test coverage is solid — it covers the happy path (build → reduce → serialize → deserialize → search), repeated reductions, and invalid input rejection. The performance data in the PR description is compelling (2.47x construction speedup).
LHT129
left a comment
There was a problem hiding this comment.
Overall this is a well-structured PR. The degree reduction capability is cleanly separated into its own translation unit, the public API surface on HGraph is minimal (3 methods), and the guard conditions in can_reduce_max_degree_unlocked are thorough. The test covers the happy path including successive reductions, serialization round-trip, and invalid input rejection.
The existing inline comments cover the main areas for improvement: parallel_for exception safety, the O(N) linear scan in rank_graph, the flatten parameter naming, and the non-persisted degree_reduction_prepared_ flag. No additional blocking issues found.
Change Type
Linked Issue
What Changed
max_degreegraph from an already builthigher-degree graph.
selected prefixes into ordinary compact graph storage.
HGraph paths.
or unsupported reductions.
deserialization, and search.
This PR intentionally provides only the HGraph primitive. AutoTune or any other consumer can call
it later, but no AutoTune dependency or orchestration is included here.
Test Evidence
make fmtmake lintmake testmake cov, run tests, and collect coverageTest details:
Performance validation used the first 500,000 vectors from SIFT1M, fp32, NSW,
ef_construction=100, 48 build threads, and degrees 16/32/64. Serialization was excluded:A separate five-repeat SIFT 20K comparison found median recall deltas from -0.0002 to +0.0112
and a maximum median projected-versus-independent latency increase of 6.9%. Searching the same
M64 artifact before and after this change produced identical recall and no measurable normal-path
latency regression.
Compatibility Impact
HGraph.
Performance and Concurrency Impact
max_degreevalues can replace repeated fullgraph construction with one construction plus inexpensive reductions. Normal build and search
paths are unchanged.
locks.
Documentation Impact
This is an internal capability with no user-facing workflow in this PR.
Risk and Rollback
9df7758a; normal HGraph paths and the serialization format areotherwise unchanged.
The reduced graph has the requested compact degree but inherits topology and hierarchy from the
larger graph. It is not byte-for-byte or topology-equivalent to a graph independently built with
the smaller
max_degree, so consumers must evaluate the reduced artifact itself.Checklist