Skip to content

chore: The from and to components of the R representation of an igraph objects use ALTREP to avoid unnecessary materialization#804

Merged
krlmlr merged 9 commits intomainfrom
from-to-altrep
May 27, 2023
Merged

chore: The from and to components of the R representation of an igraph objects use ALTREP to avoid unnecessary materialization#804
krlmlr merged 9 commits intomainfrom
from-to-altrep

Conversation

@Antonov548
Copy link
Copy Markdown
Contributor

@Antonov548 Antonov548 commented May 21, 2023

Implement ALTREP for igraph_t_idx_from and igraph_t_idx_to (requires R 4.0)

#787

@Antonov548 Antonov548 requested a review from krlmlr May 21, 2023 13:00
@Antonov548
Copy link
Copy Markdown
Contributor Author

Seems need to have two different altrep classes for from and to since it's accessing different members in graph. Or change altrep to store pointer to from or to member in graph, but not to enviorement.

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented May 21, 2023

Two classes seem fine for me. I'll review when the checks have finished.

@Antonov548 Antonov548 marked this pull request as ready for review May 21, 2023 14:01
@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented May 21, 2023

I was wondering how this even compiles with oldrel-4... Turns out we only need R >= 3.6, perhaps even R >= 3.5 would be sufficient.

Can you please add verbosity, triggered by an option, so that we can test materialization? Similarly to https://github.com/duckdb/duckdb/blob/85989198104c893f302e2fff6fae6bba65156af8/tools/rpkg/src/reltoaltrep.cpp#L65-L68.

@Antonov548
Copy link
Copy Markdown
Contributor Author

I was wondering how this even compiles with oldrel-4... Turns out we only need R >= 3.6, perhaps even R >= 3.5 would be sufficient.

Yes, it interesting. I have added igraph.verbose option.

@Antonov548
Copy link
Copy Markdown
Contributor Author

Antonov548 commented May 22, 2023

I think we can add this option to the test which is now failing: #806

Copy link
Copy Markdown
Contributor

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. As discussed, can you please extend the logging to creation and destruction of igraph objects triggered from the R code?

Comment thread src/rinterface_extra.c Outdated
Comment thread src/rinterface_extra.c Outdated
if (data == R_NilValue) {
SEXP option=Rf_GetOption(Rf_install("igraph.verbose"), R_BaseEnv);
if (option != R_NilValue && !Rf_isNull(option) && LOGICAL_ELT(option, 0) == 1) {
Rprintf("Materializing 'from' vector.\n");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R_ShowMessage() is slightly better here.

Co-authored-by: Kirill Müller <krlmlr@users.noreply.github.com>
@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented May 25, 2023

@Antonov548: can you please extend the logging to creation and destruction of igraph objects triggered from the R code?

@Antonov548
Copy link
Copy Markdown
Contributor Author

Antonov548 commented May 25, 2023

@Antonov548: can you please extend the logging to creation and destruction of igraph objects triggered from the R code?

Added logs for such events of pointer: make, free, restore.

Also found that rigraph already has function to make logs with option verbose.

library(igraph)

igraph_options(verbose = TRUE)

g <- make_ring(3, directed = TRUE)
gs <- unserialize(serialize(g, NULL))

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented May 27, 2023

Thanks, great! We can work on enhancing the messages by printing which object is saved/restored, but this is good for now.

options(conflicts.policy = list(warn = FALSE))
library(igraph)

igraph_options(verbose = TRUE)

g <- make_ring(3, directed = TRUE)
#> Make graph external pointer.
#> Materializing 'from' vector.
#> Materializing 'to' vector.
gs <- unserialize(serialize(g, NULL))
gs
#> Restore graph external pointer.
#> Make graph external pointer.
#> IGRAPH 7eb8913 D--- 3 3 -- Ring graph
#> + attr: name (g/c), mutual (g/l), circular (g/l)
#> + edges from 7eb8913:
#> [1] 1->2 2->3 3->1

Created on 2023-05-27 with reprex v2.0.2

@krlmlr krlmlr changed the title chore: altrep vector for from to chore: The from and to components of the R representation of an igraph objects use ALTREP to avoid unnecessary materialization May 27, 2023
@krlmlr krlmlr merged commit a49ef8c into main May 27, 2023
@krlmlr krlmlr deleted the from-to-altrep branch May 27, 2023 08:58
@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented May 27, 2023

Thanks!

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators May 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants