Skip to content

perf: Batch-construct vertex sequence lists via create_vs_list() - #2887

Open
schochastics wants to merge 1 commit into
perf/vs-shared-weakreffrom
perf/vs-create-vs-list
Open

perf: Batch-construct vertex sequence lists via create_vs_list()#2887
schochastics wants to merge 1 commit into
perf/vs-shared-weakreffrom
perf/vs-create-vs-list

Conversation

@schochastics

@schochastics schochastics commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Split 2 of 4 out of #2696, per #2696 (comment) — this is the requested lapply() -> create_vs_list() step.

Base: #2886 (review that first; this PR's diff is only the top commit). Next in the stack: #2888.

What changed

37 call sites built lists of vertex sequences with

lapply(res, unsafe_create_vs, graph = graph, verts = V(graph))

which re-reads the graph reference, the graph id and the name source out of verts on every single object. create_vs_list(graph, idx_list) hoists all of that per-graph work out of the loop, so each sequence costs one as.integer(), one name subset and one attributes<-.

  • The VERTEXSET_LIST OUTCONV template in tools/stimulus/types-RR.yaml now emits create_vs_list(graph, res), and the generated R/aaa-*.R files are updated to match (27 sites).
  • Hand-written call sites updated in cliques.R, cohesive.blocks.R, components.R, conversion.R, interface.R, paths.R, structural-properties.R (10 sites).
  • unsafe_create_vs() stays as the single-object form.

max_cliques(sample_gnp(200, 0.16)) on a named graph: 7.8 ms -> 5.6 ms (24.7 ms on main).

Why it matters beyond the numbers

This is the enabling refactor. It gives "turn this list of ID vectors into a list of vertex sequences" a single named entry point, so the construction loop can later be moved wholesale — into C in #2888, and behind lazy names in #2696 — without touching any of the 37 call sites again. Both of those PRs change only the body of this one function.

Scope

Edge sequences are deliberately left alone: simple_es_index() already propagates the shared weak reference from a single E(graph), so unsafe_create_es() does not pay the per-object cost. An create_es_list() batch form remains a follow-up.

Correctness

Output, names, NA handling and graph recovery are unchanged — this is a pure refactor of where the per-graph lookups happen.

Full suite: FAIL 0 | WARN 0 | SKIP 7 | PASS 9299.

Functions returning many vertex sequences used
`lapply(res, unsafe_create_vs, graph = graph, verts = V(graph))`, which
re-read the graph reference, graph id and name source from `verts` on
every object. Add `create_vs_list(graph, idx_list)`, which hoists all of
that per-graph work out of the loop and builds each sequence with a
single `attributes<-`, so per-object cost drops to an integer coercion,
a name subset and one attribute set.

The `VERTEXSET_LIST` OUTCONV template in tools/stimulus/types-RR.yaml now
emits `create_vs_list(graph, res)`; the generated R/aaa-*.R files are
updated to match (27 sites), along with the hand-written call sites in
cliques.R, cohesive.blocks.R, components.R, conversion.R, interface.R,
paths.R and structural-properties.R (10 sites). `unsafe_create_vs()`
stays as the single-object form.

Beyond the speedup this gives the construction loop a single named entry
point, so it can later be moved wholesale (e.g. into C) without touching
any call site.

Edge sequences are left as-is: simple_es_index() already propagates the
shared weak reference from a single E(graph), so unsafe_create_es() does
not pay the per-object cost. An es batch form remains a follow-up.

max_cliques(sample_gnp(200, 0.16)) on a named graph: ~7.8ms -> ~5.6ms.
Output, 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>
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if dec29d7 is merged into perf/vs-shared-weakref:

  • ✔️E_named: 211ms -> 214ms [-0.56%, +3.17%]
  • ✔️V_named: 428ms -> 429ms [-0.05%, +0.66%]
  • ✔️as_adjacency_matrix: 241ms -> 240ms [-0.95%, +0.1%]
  • ✔️as_biadjacency_matrix: 256ms -> 256ms [-0.5%, +0.64%]
  • ✔️as_data_frame_both: 261ms -> 261ms [-0.84%, +0.51%]
  • ✔️as_long_data_frame: 208ms -> 208ms [-0.81%, +0.39%]
  • ✔️es_attr_filter: 244ms -> 243ms [-0.64%, +0.01%]
  • ✔️graph_from_adjacency_matrix: 271ms -> 271ms [-0.42%, +0.45%]
  • ✔️graph_from_data_frame: 273ms -> 274ms [-0.32%, +0.8%]
  • ✔️head_of_named: 182ms -> 182ms [-0.72%, +0.94%]
  • 🚀max_cliques_named: 68.6ms -> 58.4ms [-15.4%, -14.33%]
  • ✔️vs_attr_filter: 331ms -> 330ms [-0.86%, +0.45%]
  • ✔️vs_by_name: 311ms -> 311ms [-0.49%, +0.32%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant