-
Notifications
You must be signed in to change notification settings - Fork 187
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
Is it actually better to run file.exists() in a loop? #2204
Comments
Benchmark: library(microbenchmark)
fake_files <- replicate(n = 10L, paste(sample(letters, 20L, TRUE), collapse = ""))
real_file <- tempfile()
file.create(real_file)
real_file_at =
function(fake_files, pos) if (pos == 0L) append(fake_files, fake_files[1L]) else append(fake_files, real_file, after=pos-1L)
loop = function(locs) for (loc in locs) if (file.exists(loc)) return(loc)
vec = function(locs) {
idx <- which(file.exists(locs))
if (length(idx) > 0L) {
locs[idx]
}
}
for (ii in seq_along(fake_files)) {
for (jj in 0:(ii + 1L)) {
locs <- real_file_at(head(fake_files, ii), jj)
message(ii + 1L, " total files, real found at ", jj)
print(microbenchmark(times = 1e4, loop(locs), vec(locs)))
}
} output:
|
We could rework the rest of the code to allow the vec = function(locs) locs[which(file.exists(locs))[1L]]
|
The gap shrunk, but the |
(it may be that |
Thanks for the thorough comparison. |
lintr/R/settings_utils.R
Lines 78 to 82 in 945c2a2
I think this is written under the assumption that typically the first/second entry will give the config, thus it's "not worth" running
file.exists()
for the full vector of possible config locations.But
file.exists()
is not all that expensive, is it? And this code is not executed 100s of times? So maybe it's simpler to just write this code in terms of checking the firstTRUE
entry on thefile.exists(<vector>)
result.Should check the result quantitatively first, though.
The text was updated successfully, but these errors were encountered: