Skip to content

Speed up is_installed() - shortcut when package is already loaded #1804

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Speed up is_installed() - shortcut when package is already loaded #1804

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions R/session.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,21 @@
is_installed <- function(pkg, ..., version = NULL, compare = NULL) {
check_dots_empty0(...)

# Fast path. Note that if `version` is supplied, this always falls through to the
# more expensive check.
if (is.null(version)) {
loaded <- lapply(pkg, function(x) {
is.character(x) && nzchar(x) && is.environment(.getNamespace(x))
})
if (all(as.logical(loaded))) {
return(TRUE)
}
}
Comment on lines +68 to +75
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a typical intended interface? is_installed(list("dplyr", "dbplyr"))

If not, we can move the checks "outside the loop" toget early exits for any of the !is.character(pkg) and !all(nzchar(pkg)) cases.

Next, why use is.environment(.getNamespace(x)) instead of isNamespaceLoaded(x)? The former is described as an "internal" (hence the leading .), the latter is more readable (and nanosecond-scale-faster).

Suggested change
if (is.null(version)) {
loaded <- lapply(pkg, function(x) {
is.character(x) && nzchar(x) && is.environment(.getNamespace(x))
})
if (all(as.logical(loaded))) {
return(TRUE)
}
}
if (is.null(version) && is.character(pkg) && all(nzchar(pkg)) && all(vapply(pkg, isNamespaceLoaded, NA))) {
return(TRUE)
}

Copy link
Member

@shikokuchuo shikokuchuo Jun 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The argument is expected to be a character vector c("dplyr", "dbplyr") rather than a list, so your suggestion would work well in that case. A list would just fail the fast path but continue, so is not disastrous.

If we make the change here, we should do the same at check_installed().

Thanks for the suggestion of isNamespaceLoaded(x). It is indeed faster as it shares the same C level code path as .getNamespace(x) but just returns a logical value rather than the environment itself.

Copy link
Member

Choose a reason for hiding this comment

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

@MichaelChirico sorry somehow I missed your entire code suggestion, so my inital reply was totally off the mark (now amended above).

Copy link
Contributor Author

@eitsupi eitsupi Jun 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the suggestions!

Would it be better to refactor this with check_installed after merging this since my intention of this PR is just to bring the same changes here as check_installed in #1780?

Copy link
Member

Choose a reason for hiding this comment

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

Yes that's fine thanks! If Lionel adopts the suggestion here, I will follow up with the corresponding change to check_installed().


info <- pkg_version_info(pkg, version = version, compare = compare)
all(detect_installed(info))
}

detect_installed <- function(info) {
# Internal mechanism for unit tests
hook <- peek_option("rlang:::is_installed_hook")
Expand Down