From c07c334bba9b21aca222f5ea8e1a50e51c0ab7c6 Mon Sep 17 00:00:00 2001 From: Aron Atkins Date: Tue, 24 Nov 2020 11:48:26 -0500 Subject: [PATCH 1/5] avoid repeated package dependency processing --- R/pkg.R | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/R/pkg.R b/R/pkg.R index 0ae056e9..ced1a159 100644 --- a/R/pkg.R +++ b/R/pkg.R @@ -184,7 +184,8 @@ getPackageRecords <- function(pkgNames, lib.loc = NULL, missing.package = error_not_installed, check.lockfile = FALSE, - fallback.ok = FALSE) + fallback.ok = FALSE, + .visited.packages = new.env(parent = emptyenv())) { project <- getProjectDir(project) local.repos <- get_opts("local.repos", project = project) @@ -192,6 +193,11 @@ getPackageRecords <- function(pkgNames, # screen out empty package names that might have snuck in pkgNames <- setdiff(pkgNames, "") + # avoid visiting other packages that have been recursively visited + pkgNames <- setdiff(pkgNames, ls(envir = .visited.packages)) + # ... and then remember that we have visited these packages. + for (pkg in pkgNames) { .visited.packages[[pkg]] <- TRUE } + if (check.lockfile) { lockfilePkgRecords <- getPackageRecordsLockfile(pkgNames, project = project) pkgNames <- setdiff(pkgNames, sapply(lockfilePkgRecords, "[[", "name")) @@ -272,7 +278,8 @@ getPackageRecords <- function(pkgNames, lib.loc = lib.loc, missing.package = missing.package, check.lockfile = check.lockfile, - fallback.ok = fallback.ok + fallback.ok = fallback.ok, + .visited.packages ) } From ad1463b13aba3426466dcf9088480c798eeaafed Mon Sep 17 00:00:00 2001 From: Aron Atkins Date: Tue, 24 Nov 2020 14:21:35 -0500 Subject: [PATCH 2/5] named arguments for future-protection --- R/pkg.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/pkg.R b/R/pkg.R index ced1a159..46197ac2 100644 --- a/R/pkg.R +++ b/R/pkg.R @@ -279,7 +279,7 @@ getPackageRecords <- function(pkgNames, missing.package = missing.package, check.lockfile = check.lockfile, fallback.ok = fallback.ok, - .visited.packages + .visited.packages = .visited.packages ) } From 1a16006a67d908ad599832614e9531758f1f8668 Mon Sep 17 00:00:00 2001 From: Aron Atkins Date: Tue, 24 Nov 2020 17:38:46 -0500 Subject: [PATCH 3/5] reimplement recursive pruning so we still produce full dependency lists --- R/pkg.R | 55 ++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 34 insertions(+), 21 deletions(-) diff --git a/R/pkg.R b/R/pkg.R index 46197ac2..67e82e6c 100644 --- a/R/pkg.R +++ b/R/pkg.R @@ -193,10 +193,15 @@ getPackageRecords <- function(pkgNames, # screen out empty package names that might have snuck in pkgNames <- setdiff(pkgNames, "") - # avoid visiting other packages that have been recursively visited - pkgNames <- setdiff(pkgNames, ls(envir = .visited.packages)) - # ... and then remember that we have visited these packages. - for (pkg in pkgNames) { .visited.packages[[pkg]] <- TRUE } + # Prior recursive steps may have already computed this package record and + # its recursive dependencies. Avoid constructing this package record. + priorPkgRecords <- c() + for (pkgName in pkgNames) { + if (exists(pkgName, envir = .visited.packages)) { + append(priorPkgRecords, get(pkgName, envir = .visited.packages)) + } + } + pkgNames <- setdiff(pkgNames, sapply(priorPkgRecords, "[[", "name")) if (check.lockfile) { lockfilePkgRecords <- getPackageRecordsLockfile(pkgNames, project = project) @@ -253,6 +258,7 @@ getPackageRecords <- function(pkgNames, # Collect the records together allRecords <- c( + priorPkgRecords, lockfilePkgRecords, srcPkgRecords, manualSrcPkgRecords, @@ -266,24 +272,31 @@ getPackageRecords <- function(pkgNames, # Now get recursive package dependencies if necessary if (recursive) { allRecords <- lapply(allRecords, function(record) { - deps <- getPackageDependencies(pkgs = record$name, - lib.loc = lib.loc, - available.packages = available) - if (!is.null(deps)) { - record$depends <- getPackageRecords( - deps, - project = project, - available, - TRUE, - lib.loc = lib.loc, - missing.package = missing.package, - check.lockfile = check.lockfile, - fallback.ok = fallback.ok, - .visited.packages = .visited.packages - ) + if (exists(record$name, envir = .visited.packages)) { + # We have already processed this package and computed its recursive + # dependencies. Avoid recursively computing its dependencies. + get(record$name, envir = .visited.packages) + } else { + # We have not already processed this package. + deps <- getPackageDependencies(pkgs = record$name, + lib.loc = lib.loc, + available.packages = available) + if (!is.null(deps)) { + record$depends <- getPackageRecords( + deps, + project = project, + available, + TRUE, + lib.loc = lib.loc, + missing.package = missing.package, + check.lockfile = check.lockfile, + fallback.ok = fallback.ok, + .visited.packages = .visited.packages + ) + } + .visited.packages[[record$name]] <- record + record } - - record }) } From 5af141c93477c2684841fa4aa7199de74e799367 Mon Sep 17 00:00:00 2001 From: Aron Atkins Date: Tue, 24 Nov 2020 18:04:43 -0500 Subject: [PATCH 4/5] correctly compute the list of previously seen packages --- R/pkg.R | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/R/pkg.R b/R/pkg.R index 67e82e6c..0b5c1a0a 100644 --- a/R/pkg.R +++ b/R/pkg.R @@ -195,13 +195,12 @@ getPackageRecords <- function(pkgNames, # Prior recursive steps may have already computed this package record and # its recursive dependencies. Avoid constructing this package record. - priorPkgRecords <- c() - for (pkgName in pkgNames) { - if (exists(pkgName, envir = .visited.packages)) { - append(priorPkgRecords, get(pkgName, envir = .visited.packages)) - } + priorPkgRecords <- dropNull(lapply(pkgNames, function(pkgName) { + get0(pkgName, envir = .visited.packages, ifnotfound = NULL) + })) + if (length(priorPkgRecords)) { + pkgNames <- setdiff(pkgNames, sapply(priorPkgRecords, "[[", "name")) } - pkgNames <- setdiff(pkgNames, sapply(priorPkgRecords, "[[", "name")) if (check.lockfile) { lockfilePkgRecords <- getPackageRecordsLockfile(pkgNames, project = project) From 2693f8b2f6ccf8e248d94a0425a1eabc97c11fb2 Mon Sep 17 00:00:00 2001 From: Aron Atkins Date: Wed, 25 Nov 2020 09:09:34 -0500 Subject: [PATCH 5/5] remove use of get0 --- R/pkg.R | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/R/pkg.R b/R/pkg.R index 0b5c1a0a..98b755b4 100644 --- a/R/pkg.R +++ b/R/pkg.R @@ -196,7 +196,11 @@ getPackageRecords <- function(pkgNames, # Prior recursive steps may have already computed this package record and # its recursive dependencies. Avoid constructing this package record. priorPkgRecords <- dropNull(lapply(pkgNames, function(pkgName) { - get0(pkgName, envir = .visited.packages, ifnotfound = NULL) + if (exists(pkgName, envir = .visited.packages)) { + get(pkgName, envir = .visited.packages) + } else { + NULL + } })) if (length(priorPkgRecords)) { pkgNames <- setdiff(pkgNames, sapply(priorPkgRecords, "[[", "name"))