Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rewrite prop() in C #395

Merged
merged 3 commits into from
Dec 18, 2023
Merged

rewrite prop() in C #395

merged 3 commits into from
Dec 18, 2023

Conversation

t-kalinowski
Copy link
Collaborator

@t-kalinowski t-kalinowski commented Dec 11, 2023

Addresses #363

@t-kalinowski
Copy link
Collaborator Author

t-kalinowski commented Dec 17, 2023

Slightly over a 10x speed-up by moving prop() to C for the most simple case.

library(S7)

foo <- new_class("foo", properties = list(xyz = class_double))
obj <- foo(1)
propr <- S7:::propr
print(plot(print(df <- bench::mark(
  c = prop(obj, "xyz"),
  r = propr(obj, "xyz")
))))
#> # A tibble: 2 × 13
#>   expression      min   median `itr/sec` mem_alloc `gc/sec` n_itr  n_gc
#>   <bch:expr> <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl> <int> <dbl>
#> 1 c          205.01ns 287.02ns  3215873.        0B      0   10000     0
#> 2 r            3.77µs   4.14µs   227757.    39.8KB     45.6  9998     2
#> # ℹ 5 more variables: total_time <bch:tm>, result <list>, memory <list>,
#> #   time <list>, gc <list>
#> Loading required namespace: tidyr

Created on 2023-12-17 with reprex v2.0.2

Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

Looks good! Do you think it's worth iterating in this PR for bit longer (including a bit of thought about how to track performance more broadly), or it's better to merge this an iterate in future PRs?

}

static inline
Rboolean inherits2(SEXP object, const char* name) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice idea!

Comment on lines +109 to +122
if(has_name(properties, name_char))
return value;

if (S7_class == R_NilValue &&
is_s7_class(object) && (
name_sym == sym_name ||
name_sym == sym_parent ||
name_sym == sym_package ||
name_sym == sym_properties ||
name_sym == sym_abstract ||
name_sym == sym_constructor ||
name_sym == sym_validator
))
return value;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it's worth implementing prop_names() and prop_exists() at the same time because that might influence the design of name validation?

@t-kalinowski
Copy link
Collaborator Author

t-kalinowski commented Dec 18, 2023

better to merge this and iterate in future PRs

I'd vote to merge now, since it's immediately beneficial, and then we can continue to iterate in future PRs (also, it's looking like prop<- is going to involve some discussion, and would probably be better in its own PR).

@hadley
Copy link
Member

hadley commented Dec 18, 2023

Sounds good. Can you please add a bullet to news? (We wouldn't normally because it's not user facing, but it'll remind us to discuss in the next WG meeting)

@hadley hadley merged commit 19fa07b into main Dec 18, 2023
12 checks passed
@hadley hadley deleted the rewrite-prop-in-c branch December 18, 2023 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants