Skip to content
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

Speedup render by avoiding repeated vector copies. #176

Merged
merged 10 commits into from
Dec 26, 2023

Conversation

plietar
Copy link
Member

@plietar plietar commented Dec 21, 2023

The Render$render method is used throughout a simulation to store data that needs outputting at the end. It does so by creating a vector per metric, large enough to fit all timesteps.

Unfortunately, each time the render method is called, the assignment into the vector actually creates a new copy, instead of modifying it in-place. For long simulations (ie. tens of thousands of timesteps), these vectors are both fairly large and get copied over and over again at each timestep, to the point where the render method dominates the execution time.

The core of the problem can easily be reproduced by recreating the same conditions of the Render class, using a vector stored inside of an environment:

e <- new.env(parent=emptyenv())
e$vector = rep(0, 10)
tracemem(e$vector)
# [1] "<0x56313d8a4c80>"
e$vector[[0]] <- 1
# tracemem[0x56313d8a4c80 -> 0x56314180ff60]:
e$vector[[1]] <- 1
# tracemem[0x56314180ff60 -> 0x563141613fc0]:
e$vector[[2]] <- 1
# tracemem[0x563141613fc0 -> 0x563141984b10]:

Here e mirrors the private environment of the Render class, as it would be constructed by R6. The tracemem call is used to trace copies of the vector. Each subsequent assignment to the vector creates a new copy of it, which can be seen as tracemem lines in the console.

I did experiment with ways of avoiding the copies without resorting to C++ code, but I couldn't find anything that wasn't made of horrible hacks.

The patch here fixes this issue by creating a C++ class, that acts as a thin wrapper around a vector. Thanks to this indirection, the vector is guarateed to have reference semantics and be modified in-place. A method exists on the wrapper class to extract the underlying vector. This does incur a copy, but is only expected to be called once (per vector), at the end of the simulation.

For large runs, this change has a dramatic effect of the performance of the end-to-end simulation. Using malariasimulation as a benchmark:

# Before the change
system.time(run_simulation(timesteps = 50000))
#    user  system elapsed
# 334.662   0.604 335.253

# Afterwards
system.time(run_simulation(timesteps = 50000))
#    user  system elapsed
# 134.270   0.008 134.271

The impact is much less noticeable are shorter runs, since the vectors being copied are much smaller (on the order of 20% improvement for a 10'000 time steps run, 5% for 5'000 time steps).

giovannic and others added 5 commits August 30, 2023 15:14
The `Render$render` method is used throughout a simulation to store data
that needs outputting at the end. It does so by creating a vector per
metric, large enough to fit all timesteps.

Unfortunately, each time the `render` method is called, the assignment
into the vector actually creates a new copy, instead of modifying it
in-place. For long simulations (ie. tens of thousands of timesteps),
these vectors are both fairly large and get copied over and over again
at each timestep, to the point where the `render` method dominates the
execution time.

The core of the problem can easily be reproduced by recreating the same
conditions of the `Render` class, using a vector stored inside of an
environment:

```r
e <- new.env(parent=emptyenv())
e$vector = rep(0, 10)
tracemem(e$vector)
# [1] "<0x56313d8a4c80>"
e$vector[[0]] <- 1
# tracemem[0x56313d8a4c80 -> 0x56314180ff60]:
e$vector[[1]] <- 1
# tracemem[0x56314180ff60 -> 0x563141613fc0]:
e$vector[[2]] <- 1
# tracemem[0x563141613fc0 -> 0x563141984b10]:
```

Here `e` mirrors the `private` environment of the `Render` class, as it
would be constructed by R6. The `tracemem` call is used to trace copies
of the vector. Each subsequent assignment to the vector creates a new
copy of it, which can be seen as tracemem lines in the console.

I did experiment with ways of avoiding the copies without resorting to
C++ code, but I couldn't find anything that wasn't made of horrible
hacks.

The patch here fixes this issue by creating a C++ class, that acts as a
thin wrapper around a vector. Thanks to this indirection, the vector is
guarateed to have reference semantics and be modified in-place. A method
exists on the wrapper class to extract the underlying vector. This does
incur a copy, but is only expected to be called once (per vector), at
the end of the simulation.

For large runs, this change has a dramatic effect of the performance of
the end-to-end simulation. Using malariasimulation as a benchmark:
```r
# Before the change
system.time(run_simulation(timesteps = 50000))
#    user  system elapsed
# 334.662   0.604 335.253

# Afterwards
system.time(run_simulation(timesteps = 50000))
#    user  system elapsed
# 134.270   0.008 134.271
```

The impact is much less noticeable are shorter runs, since the vectors
being copied are much smaller (on the order of 20% improvement for a
10'000 time steps run, 5% for 5'000 time steps).
Copy link

codecov bot commented Dec 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4970334) 95.86% compared to head (c9cdee3) 96.28%.
Report is 12 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #176      +/-   ##
==========================================
+ Coverage   95.86%   96.28%   +0.41%     
==========================================
  Files          34       36       +2     
  Lines        1717     1722       +5     
==========================================
+ Hits         1646     1658      +12     
+ Misses         71       64       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/render_vector.cpp Outdated Show resolved Hide resolved
R/render.R Outdated Show resolved Hide resolved
R/render.R Outdated Show resolved Hide resolved
src/render_vector.cpp Outdated Show resolved Hide resolved
@@ -366,7 +366,7 @@ RcppExport SEXP _individual_dummy() {
if (rcpp_isError_gen) {
SEXP rcpp_msgSEXP_gen = Rf_asChar(rcpp_result_gen);
UNPROTECT(1);
Rf_error(CHAR(rcpp_msgSEXP_gen));
Rf_error("%s", CHAR(rcpp_msgSEXP_gen));
Copy link
Member Author

Choose a reason for hiding this comment

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

This was introduced by running against a prerelease of Rcpp to avoid a CI failure, as explained in RcppCore/Rcpp#1287 (comment).

We only need the pre-release to generate this file. Users of the package don't need it.

@plietar plietar requested a review from giovannic December 21, 2023 22:31
@@ -455,5 +467,5 @@ variable_resize <- function(variable) {

# Register entry points for exported C++ functions
methods::setLoadAction(function(ns) {
.Call('_individual_RcppExport_registerCCallable', PACKAGE = 'individual')
.Call(`_individual_RcppExport_registerCCallable`)
Copy link
Member Author

Choose a reason for hiding this comment

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

This change comes from an Rcpp change at RcppCore/Rcpp#1256

@plietar plietar requested a review from richfitz December 21, 2023 22:44
Copy link
Member

@giovannic giovannic left a comment

Choose a reason for hiding this comment

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

This is great thanks!

I would suggest you to have a look at (and extend if you want?) the existing microbenchmarks. They run a variety of scenarios to show the effects of an improvement in a variety of scenarios, here's an example of how it can be helpful.

@plietar
Copy link
Member Author

plietar commented Dec 22, 2023

Thanks. I've added two microbenchmarks, one which does a single render() call and one which calls it as many times as there are timesteps, mimicking the behaviour of a full simulation.

Rplots.pdf

@giovannic giovannic changed the base branch from master to dev December 26, 2023 13:01
@giovannic giovannic merged commit e4a24e7 into mrc-ide:dev Dec 26, 2023
7 checks passed
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.

3 participants