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

Implements avoid loading rlang where not required #187

Merged
merged 7 commits into from
Apr 29, 2024
Merged

Conversation

shikokuchuo
Copy link
Member

As per the suggestion by Winston Chang.

  • Implements required weakref functions for internal package use only.
  • conditions rlang::as_function() usage only where !is.function()

In addition:

  • uses native symbol registration - this is a real speedup, apart from just being 'recommended practice'.
  • removes blanket Rcpp import, just the selective import is fine

Closes #186.

@wch
Copy link
Member

wch commented Apr 29, 2024

This is great. Can you also add an entry to the NEWS.md file?

I'm curious: how much of a speed improvement does the symbol registration provide?

There could be another boost from removing the Rcpp dependency and replace it with cpp11, but that would be a more difficult project.

R/later.R Show resolved Hide resolved
R/later.R Show resolved Hide resolved
@shikokuchuo
Copy link
Member Author

Have added the comments and a NEWS item.

The saving from native symbol registration is every time there is a .Call(). As dynamic lookup involved a string search... I haven't benchmarked but remember it being slow. I contributed this change to the torch mlverse package some time ago, and as it is so atomic there (nearly every operation is an underlying tensor operation hence a .Call), the impact was not negligible.

@wch wch merged commit 968542a into r-lib:main Apr 29, 2024
@wch
Copy link
Member

wch commented Apr 29, 2024

Thanks!

@shikokuchuo shikokuchuo deleted the dev branch April 29, 2024 20:29
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.

Peformance: dependency on rlang
2 participants