Skip to content

Conversation

@wlandau
Copy link
Contributor

@wlandau wlandau commented Nov 4, 2018

Using base64url in encode64() decreases overhead. Relevant: #43, #44. Benchmarking code:

library(microbenchmark)
library(storr)

n <- 1000

cache_mangle <- storr::storr_rds(tempfile(), mangle_key = TRUE)
cache_no_mangle <- storr::storr_rds(tempfile(), mangle_key = FALSE)

for (i in seq_len(n)){
  cache_mangle$set(key = as.character(i), value = i)
  cache_no_mangle$set(key = as.character(i), value = i)
}

overhead <- function(cache, n){
  for (i in seq_len(n)){
    cache$get(key = as.character(i))
  }
}

benchmarks <- microbenchmark(
  overhead(cache_mangle, n),
  overhead(cache_no_mangle, n)
)

print(benchmarks)

Benchmarks with the native implementation:

#> Unit: milliseconds
#>                          expr      min        lq      mean    median
#>     overhead(cache_mangle, n) 98.03836 104.64194 108.22292 107.92887
#>  overhead(cache_no_mangle, n) 62.32374  65.64005  67.20944  66.88925
#>         uq       max neval cld
#>  111.33038 136.31826   100   b
#>   68.23469  95.06846   100  a

Benchmarks with this PR.

#> Unit: milliseconds
#>                          expr      min       lq     mean   median       uq
#>     overhead(cache_mangle, n) 83.93953 90.30987 94.06254 94.10564 97.07974
#>  overhead(cache_no_mangle, n) 62.71232 65.66816 68.68043 68.64221 70.61832
#>        max neval cld
#>  123.58915   100   b
#>   95.26984   100  a

Do you think this speedup is enough? I was hoping for more of an improvement.

Benchmarks if encode64() simply calls base64_urlencode() (without compatibility measures for char62, char63, or pad):

#> Unit: milliseconds
#>                          expr      min       lq     mean   median       uq
#>     overhead(cache_mangle, n) 76.86742 82.28243 84.44084 84.30136 86.61499
#>  overhead(cache_no_mangle, n) 63.01152 66.57724 69.32338 68.83174 70.57834
#>        max neval cld
#>   93.08793   100   b
#>  106.44841   100  a

@codecov-io
Copy link

codecov-io commented Nov 4, 2018

Codecov Report

Merging #87 into master will decrease coverage by 0.17%.
The diff coverage is 84.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #87      +/-   ##
==========================================
- Coverage   99.91%   99.74%   -0.18%     
==========================================
  Files          15       15              
  Lines        1179     1176       -3     
==========================================
- Hits         1178     1173       -5     
- Misses          1        3       +2
Impacted Files Coverage Δ
R/base64.R 93.93% <84.61%> (-6.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2750823...7ad1f56. Read the comment docs.

@wlandau
Copy link
Contributor Author

wlandau commented Nov 4, 2018

Given the benchmarks and the potential package dependency on base64url, I actually like #88 more than this PR.

@richfitz richfitz self-requested a review November 4, 2018 09:17
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.

2 participants