perf: Share the graph weak-reference across constructed sequences - #2886
Open
schochastics wants to merge 3 commits into
Open
perf: Share the graph weak-reference across constructed sequences#2886schochastics wants to merge 3 commits into
schochastics wants to merge 3 commits into
Conversation
Constructing a vertex/edge sequence attached a graph reference per object via add_vses_graph_ref(), which calls .Call(Rx_igraph_copy_env), .Call(Rx_igraph_make_weak_ref) and .Call(Rx_igraph_get_graph_id) for every object. For functions returning many sequences (e.g. max_cliques returning tens of thousands) this dominated construction time -- profiling showed it was ~75% of the cost, far more than name building. The weak reference's key is the graph's environment, which is identical for every sequence of a graph (Rf_duplicate() is a no-op on an environment, so get_vs_ref() returns the same env each call). A single shared weak reference is therefore semantically identical to one per object: while the graph is alive the reference resolves, and once the graph is released the (weak) reference reports it gone -- verified that get_vs_graph() still returns NULL after rm(graph); gc(). simple_es_index() already propagates env/graph from its input, so edge construction only needed the redundant per-object add_vses_graph_ref() call removed. simple_vs_index() now propagates env/graph the same way, and unsafe_create_vs()/unsafe_create_es() rely on that propagation. The existing `lapply(res, unsafe_create_vs, graph = graph, verts = V(graph))` call sites then share the single weak reference built by V()/E() with no call-site or codegen changes. max_cliques(sample_gnp(500, 0.15)) on a named graph: ~338ms -> ~200ms. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two construction-cost reductions on top of the shared weak reference: * simple_vs_index() now sets names/class/env/graph in a single `attributes<-` call instead of separate `attr<-`/`class<-` assignments. Each incremental assignment shallow-copies the vector, and that copying dominated when building many sequences. * unsafe_create_vs() no longer routes through simple_vs_index(). Its `idx` are vertex IDs from C and `verts` is always the full V(graph), so `verts[idx]` just reproduces `idx`; we now use the IDs directly as the (integer) payload and subset the names off `verts`, avoiding a full copy of V(graph) per object. Payload type (integer), names, NA handling and graph recovery are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Add a benchmark group exercising the sequence-construction path on named graphs, where building the `names`/`vnames` attribute and attaching the graph reference dominate: max_cliques (thousands of vertex sequences), head_of over every edge, and V()/E() on a large named graph. Repeat counts follow the convention documented at the top of the script: warm up in the setup block, then loop a literal number of times so the measured region lands near 100 ms on the base branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 4, 2026
Contributor
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 1220792 is merged into main:
|
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.
Split 1 of 4 out of #2696, per #2696 (comment).
Base:
main. Next in the stack: #2887.Why this one is not in the requested split
The review asked for three PRs (
create_vs_list(), then C, then ALTREP). Profiling the split showed a fourth piece that belongs first, because it is where most of the speedup actually comes from and it is independent of all three: sequence construction was minting a graph weak-reference per object and setting attributes one at a time.max_cliques(sample_gnp(200, 0.16)), named graphmainThat is ~3.2× before a single line of
create_vs_list()or C code exists.What changed
1. One shared weak reference per graph instead of one per sequence.
add_vses_graph_ref()runs three.Call()s (Rx_igraph_copy_env,Rx_igraph_make_weak_ref,Rx_igraph_get_graph_id) for every object it touches. Functions returning thousands of sequences paid that per sequence — profiling put it at ~75% of construction time.The weak reference's key is the graph's environment, which is identical for every sequence of a graph (
Rf_duplicate()is a no-op on an environment, soget_vs_ref()hands back the same env each call). A single shared reference is therefore semantically identical to one per object: while the graph is alive the reference resolves; once the graph is released the reference reports it gone.simple_es_index()already propagatedenv/graphfrom its input, so the edge path only needed the redundant per-object call removed.simple_vs_index()now propagates them the same way, so the existinglapply(res, unsafe_create_vs, graph = graph, verts = V(graph))call sites share the one reference minted byV(graph)— no call-site or codegen changes.2. Attributes set in one pass. Each incremental
attr<-/class<-shallow-copies the vector.simple_vs_index()now setsnames/class/env/graphin a singleattributes<-.3.
unsafe_create_vs()skips a redundant copy. Itsidxare vertex IDs straight from C andvertsis always the fullV(graph), soverts[idx]just reproducesidx. It now uses the IDs directly as the integer payload and subsets the names offverts, instead of copyingV(graph)per object.Correctness
Payload type (integer), names, NA handling and graph recovery are unchanged. Verified that
get_vs_graph(seq)still returnsNULLafterrm(graph); gc()— the shared reference is still weak.Full suite: FAIL 0 | WARN 0 | SKIP 7 | PASS 9299.
Also here
Touchstone group #5 covering the construction path on named graphs (
max_cliques_named,head_of_named,V_named,E_named), written in the warm-up-then-loop style the script's header requires, with literal repeat counts targeting ~100 ms onmain.