Dev #177
Merged
Dev #177
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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).
Speedup render by avoiding repeated vector copies.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #177 +/- ##
==========================================
+ 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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hey @plietar, just wanted to make sure you're happy with how you're listed in Authors before I release.