perf: Build the whole vertex-sequence list in one C pass - #2888
Open
schochastics wants to merge 1 commit into
Open
perf: Build the whole vertex-sequence list in one C pass#2888schochastics wants to merge 1 commit into
schochastics wants to merge 1 commit into
Conversation
create_vs_list() drove the per-element work (lapply closure, as.integer, name subset, attributes<-) from R, repeated once per sequence -- the dominant remaining cost when functions like max_cliques() return thousands of igraph.vs objects. Move that loop into Rx_igraph_vs_list() in rinterface_extra.c. R now only builds V(graph) once (to mint the shared weak reference and graph id) and hands the pieces to C. Each element gets a fresh integer payload (guarded against coerceVector aliasing so the caller's vectors are never mutated), the corresponding names when the graph is named, the shared env/graph attributes, and the igraph.vs class. max_cliques(sample_gnp(200, 0.16)) on a named graph: ~5.6ms -> ~2.65ms, and ~24.7ms -> ~2.65ms over the three PRs so far. Correctness verified: integer payloads with identical values/names, no names attribute on unnamed graphs, NA/out-of-range IDs map to NA_STRING, double inputs coerced, inputs unmutated, and the shared weakref still releases the graph after rm()+gc(). Full suite passes; clean under gctorture(TRUE). Also adds two touchstone benchmarks (ego_order2_named, all_simple_paths_named) covering the batch-construction path. cpp11::cpp_register() added only the Rx_igraph_vs_list registration. 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 dd0e928 is merged into perf/vs-create-vs-list:
|
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 3 of 4 out of #2696, per #2696 (comment) — this is the requested "C implementation of
create_vs_list()without ALTREP" step.Base: #2887 (review #2886 and #2887 first; this PR's diff is only the top commit). Last in the stack: #2696.
What changed
After #2887,
create_vs_list()still drove the per-element work from R — anlapplyclosure call,as.integer(), a name subset andattributes<-, once per sequence. That is the dominant remaining cost when a function returns thousands ofigraph.vsobjects.Rx_igraph_vs_list()inrinterface_extra.cdoes the whole loop in one pass. R only buildsV(graph)once (that is what mints the shared weak reference and graph id) and hands the pieces to C:Each element gets a fresh integer payload, the corresponding names when the graph is named, the shared
env/graphattributes, and theigraph.vsclass. Names are built eagerly here — no ALTREP, as requested. That is the only thing #2696 changes on top.Note the payload is duplicated when
Rf_coerceVector()is a no-op, so a caller-owned integer vector is never mutated in place.max_cliques(sample_gnp(200, 0.16))on a named graph: 5.6 ms -> 2.65 ms.Where that leaves the stack
mainmaincreate_vs_list())So the three merged PRs get essentially the whole win without any ALTREP. This is the point at which #2696's "if needed" can be judged on its own merits.
Correctness
Verified against the
return.vs.es = FALSEnumeric path:namesattribute at all on unnamed graphs;NAand out-of-range IDs map toNA_STRING;get_vs_graph(seq)isNULLafterrm(graph); gc().Compiles without new warnings, and clean under
gctorture(TRUE).Full suite: FAIL 0 | WARN 0 | SKIP 7 | PASS 9299.
Also here
cpp11::cpp_register()added only theRx_igraph_vs_listregistration insrc/cpp11.cpp. Two touchstone benchmarks for the batch path:ego_order2_namedandall_simple_paths_named.