perf: Lazy ALTREP names for vertex/edge sequences - #2696
Conversation
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 21f3d4b is merged into main:
|
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 79fadf6 is merged into main:
|
| consistent API. | ||
| } | ||
| \section{Related documentation in the C library}{ | ||
| \href{https://igraph.org/c/html/0.10.17/igraph-Structural.html#igraph_biconnected_components}{\code{biconnected_components()}}, \href{https://igraph.org/c/html/0.10.17/igraph-Basic.html#igraph_vcount}{\code{vcount()}}, \href{https://igraph.org/c/html/0.10.17/igraph-Basic.html#igraph_edges}{\code{edges()}}, \href{https://igraph.org/c/html/0.10.17/igraph-Basic.html#igraph_get_eids}{\code{get_eids()}}, \href{https://igraph.org/c/html/0.10.17/igraph-Basic.html#igraph_ecount}{\code{ecount()}} |
There was a problem hiding this comment.
should this be part of this diff?
There was a problem hiding this comment.
not sure why this is a diff here
There was a problem hiding this comment.
maybe it disappears when rerunning document
| benchmark_run( | ||
| expr_before_benchmark = { | ||
| library(igraph) | ||
| set.seed(42) |
There was a problem hiding this comment.
why don't we use withr here? I'm genuinely curious.
There was a problem hiding this comment.
Claude not a fan, maybe? I nudge to it 😆
There was a problem hiding this comment.
Ah I think because these scripts are supposed to be quite dependency free? All that is needed is touchstone and igraph
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 07598b9 is merged into main:
|
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if bd02062 is merged into main:
|
|
Conflicting now. |
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 63ddefd is merged into main:
|
krlmlr
left a comment
krlmlr
left a comment
There was a problem hiding this comment.
Perhaps use an existing ALTREP class from sparsevctrs or stringfish? What's the performance tradeoff?
.Internal(inspect(letters))
#> @11ae9a930 16 STRSXP g1c7 [MARK,REF(1)] (len=26, tl=0)
#> @12ba04b40 09 CHARSXP g1c1 [MARK,REF(452),gp=0x61] [ASCII] [cached] "a"
#> @12cb694c8 09 CHARSXP g1c1 [MARK,REF(12),gp=0x61] [ASCII] [cached] "b"
#> @12b013488 09 CHARSXP g1c1 [MARK,REF(426),gp=0x61] [ASCII] [cached] "c"
#> @12baba488 09 CHARSXP g1c1 [MARK,REF(6),gp=0x61] [ASCII] [cached] "d"
#> @12b059a10 09 CHARSXP g1c1 [MARK,REF(562),gp=0x61] [ASCII] [cached] "e"
#> ...
.Internal(inspect(sparsevctrs::as_sparse_character(letters)))
#> @10d3a45a8 16 STRSXP g0c0 [] sparsevctrs_altrep_sparse_string (materialized=F, length=26)
.Internal(inspect(sparsevctrs::as_sparse_character(letters)[1:3]))
#> @10d3dc2a8 16 STRSXP g0c0 [] sparsevctrs_altrep_sparse_string (materialized=F, length=3)Created on 2026-07-09 with reprex v2.1.1
Re: reusing an existing ALTREP class (sparsevctrs / stringfish)Short version: neither fits, because this class isn't really a "string vector" — it's an indexed gather-view over a shared parent vector, which is a different ALTREP pattern than what either library implements. What this class actually is
Why sparsevctrs doesn't fit
Why stringfish doesn't fit
Dependency cost (independent of fit)igraph is Summary
Recommendation: keep the custom class. The abstraction we need — a lazy indexed view into a shared vector — is genuinely bespoke; neither library exposes it, and both would regress exactly the construction-time and subsetting costs. The closest published analog is R's own |
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 0b9c07c is merged into main:
|
|
Let's split this in three separate PRs:
|
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if b9e1823 is merged into main:
|
|
Did we agree to split this PR? |
|
Yes, I just did not have the time yet to do anything |
A vertex/edge sequence's `names` attribute is now an instance of an `igraph_lazy_names` ALTREP string class instead of a materialized character vector. It holds a reference to the graph's full name vector plus a 1-based index into it, and only builds the actual names when an element is touched (printing, named indexing, `as_ids()`). Subsetting stays lazy too: `Extract_subset` composes the indices in O(1) rather than copying a slice of the names. `V()`/`E()` build their names through `lazy_index_names()`, and `Rx_igraph_vs_list()` attaches the ALTREP directly instead of eagerly subsetting the name vector. On top of the shared weak reference, create_vs_list() and the C construction loop, this is a smaller and more selective win than the earlier layers -- it only pays off where the name vectors themselves are large or never read: ego_order2_named 4.43ms -> 3.13ms (-29%) vs_subset_positional 0.111ms -> 0.095ms (-14%) max_cliques_named 2.67ms -> 2.67ms (unchanged) all_simple_paths_named 1.94ms -> 1.92ms (unchanged) DATAPTR_RO/DATAPTR_OR_NULL are used rather than DATAPTR, which is non-API as of R 4.5. Correctness verified: integer payloads with identical values and names, no names attribute on unnamed graphs, NA/out-of-range IDs map to NA_STRING, inputs unmutated, and the shared weakref still releases the graph after rm()+gc(). Full suite passes; clean under gctorture(TRUE). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bce51e9 to
04bf69b
Compare
schochastics
changed the title
|
Split done, per @krlmlr's #2696 (comment). The stack:
Two notes on the split as requested: There is a fourth PR, and it goes first. Profiling the split showed the largest single win — the shared weak reference plus one-pass attribute setting — is independent of all three requested steps and accounts for most of the total. It is #2886, ~15 lines in The "if needed" on ALTREP resolves to "mostly not". Once the construction loop is in C, eager names are just Each PR was verified independently: full suite |
|
These four are now a native GitHub stack (stack #2890), so the stack map is on each PR and #2886–#2888 can be merged from the stack UI — retargeting and rebasing of the layers above cascade automatically, including under this repo's squash merges. The "Base:" lines in the descriptions are just there for reading order. |
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 04bf69b is merged into perf/vs-list-c:
|
3 participants
Split 4 of 4, per #2696 (comment) — this is the "if needed, ALTREP on top" step. Rebased onto the stack, so the diff here is now only the ALTREP layer.
Base: #2888. Review order: #2886 -> #2887 -> #2888 -> this.
Part of #2695, relates to #1652.
Honest answer to "if needed": mostly not
With the first three PRs merged, ALTREP no longer moves the flagship case at all. Measured head-to-head (#2888 vs this PR, same machine, warm, 20-30 iterations):
max_cliques_namedmax_cliques_sizes_namedall_simple_paths_namedego_order2_namedvs_subset_positionalThe reason
max_cliques()sees nothing: once the construction loop is in C (#2888), building the names eagerly is just a run ofSET_STRING_ELTagainst already-internedCHARSXPs. No allocation of new strings, no R-level copy. Laziness has almost nothing left to save.Where it does help is where the name vectors are large relative to the work:
ego(order = 2)builds 2000 sequences whose neighborhoods are a large fraction of the graph, and positional subsetting of a 50k-vertex sequence composes indices in O(1) instead of copying 10k names.So this is a real but narrow win, and it is the one layer of the four that carries ongoing maintenance cost: a custom ALTREP class with
Length/Dataptr/Dataptr_or_null/Extract_subset/Eltmethods, and a dependence on R's ALTREP API staying stable. I'd be fine closing this if the maintenance cost isn't worth 29% onego()— #2886-#2888 already deliver 9.3× on the case #1652 was actually about.What changed
A sequence's
namesattribute becomes an instance of anigraph_lazy_namesALTREP string class instead of a materialized character vector.data1holdslist(source, idx)— the graph's full name vector, shared by reference across every sequence of that graph, plus a 1-based index into it;data2caches the materializedSTRSXPonce anything reads it.V()/E()build their names throughlazy_index_names().Rx_igraph_vs_list()attaches the ALTREP directly instead of eagerly subsetting.Extract_subsetreturns a fresh lazy vector with composed indices, so subsetting a sequence never materializes; it falls back to the default for index types it does not handle.DATAPTR_RO/DATAPTR_OR_NULLare used rather thanDATAPTR, which is non-API as of R 4.5.Correctness
Same checks as #2888, re-run here: integer payloads with identical values and names against the
return.vs.es = FALSEpath, nonamesattribute at all on unnamed graphs,NA/out-of-range IDs map toNA_STRING, double ID vectors coerced, caller inputs unmutated, andget_vs_graph(seq)stillNULLafterrm(graph); gc().Compiles without new warnings, clean under
gctorture(TRUE).Full suite: FAIL 0 | WARN 0 | SKIP 7 | PASS 9299.
Scope
Vertex sequences and the
namesof edge sequences. Edgevnames("tail|head") are still built eagerly; a lazyvnamesand acreate_es_list()batch form remain follow-ups.On
return.vs.esThe original framing of this PR still holds, and is now better supported. #1652 was blocked on an 8x gap between the default and
return.vs.es = FALSEthat stibu81 called "unusable". After #2886-#2888 that gap is ~1.15x. The option's main justification is gone, so it can be deprecated and left inert rather than urgently removed — a separate decision, untouched by this stack.