Bioconductor / BiocParallel

Bioconductor facilities for parallel evaluation
https://bioconductor.org/packages/BiocParallel
65 stars 29 forks source link

Performance (speed) degradation in MulticoreParam with default force.G = TRUE #238

Closed mtmorgan closed 1 year ago

mtmorgan commented 1 year ago

This support site question https://support.bioconductor.org/p/9140528/#9148511 reports poor performance with default MulticoreParam(). Using Rprof(); ...; Rprof(NULL); summaryRprof() suggests the culprit is garbage collection, and setting force.GC = FALSE resolves the problem. The original motivation for forcing garbage collection and solution are discussed in https://github.com/Bioconductor/BiocParallel/pull/124

Jiefei-Wang commented 1 year ago

I always use force.GC=FALSE in practice, but I use this because I know force.GC has performance penalties, and most times I do not need to do GC. For normal users, they may know what this option does but do not understand why they need it(in other words, they do not know when they should turn it off).

I do not use MulticoreParam a lot so this is not a problem for me, but I feel that force.GC should not even be an option. If a user wants to do a GC at the end of the function, just add gc() at the end. It would not take too much effort. Having this option not only complicates the structure of BiocParallal but also adds additional learning costs (which looks unnecessary). Therefore, instead of changing the default to force.GC=FALSE, I suggest removing this option in our devel version to avoid this issue.

DarwinAwardWinner commented 1 year ago

If a user wants to do a GC at the end of the function, just add gc() at the end.

This is not trivial to do if you're calling a package function. You end up having to define a wrapper function that runs the real function, stores the result, calls gc(), and finally returns the result. It's not hard, but it hurts readability a lot, and novice R users would probably never think of it. Compare:

bplapply(1:10, sqrt)
bplapply(1:10, function(x) { res <- sqrt(x); gc(); res; })

I suppose you could define a gc wrapper factory, something like:

gc_after <- function(FUN) {
  function(...) {
    res <- FUN(...)
    gc()
    res
  }
}
bplapply(1:10, gc_after(sqrt))

Having the force.GC argument is helpful (even if the default is false) because it signals to the user that this might be something useful for them to do (and in some cases it is useful, even if it's an ugly workaround). If we remove the argument, perhaps we should add something to the documentation mentioning that if the user encounters memory usage issues, they can force GC using any of the above methods?

mtmorgan commented 1 year ago

Thanks for the commentary.

I kept force.GC= because there are common situations (users of package making use of bplapply()) where it would be challenging to enforce garbage collection.

I changed the default, rationalizing that the first use is for speed gains, and that memory management is secondary and applies to a subset of problems.