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

Switch Bitset from an R6 class to a named list. #186

Merged
merged 6 commits into from
Feb 28, 2024

Conversation

plietar
Copy link
Member

@plietar plietar commented Feb 21, 2024

R6 object have significant overhead, especially when instantiating the objects. While this is acceptable for long-lived objects such as events or variables, bitsets are created and destroyed very regularly during simulations.

We can replace our use of an R6 class for Bitset with named lists that are intended to look and feel just like the original API, but which significant performance improvement. The reference semantics provided by R6 don't matter in our case, since all mutability happens behind the external pointer.

On malariasimulation, I get a 30-35% performance improvement when using this new implementation of Bitset on population sizes under 10k, and about 10% speedup at 100k.

The object-oriented named list based interface still adds a bit of overhead compared to using the externalptr and Rcpp functions directly, but doing so requires intrusive changes in the use site.

R6 object have significant overhead, especially when instantiating the
objects. While this is acceptable for long-lived objects such as events
or variables, bitsets are created and destroyed very regularly during
simulations.

We can replace our use of an R6 class for `Bitset` with named lists that
are intended to look and feel just like the original API, but which
significant performance improvement. The reference semantics provided by
R6 don't matter in our case, since all mutability happens behind the
external pointer.

On malariasimulation, I get a 30-35% performance improvement when using
this new implementation of Bitset on population sizes under 10k, and
about 10% speedup at 100k.

The object-oriented named list based interface still adds a bit of
overhead compared to using the externalptr and Rcpp functions directly,
but doing so requires intrusive changes in the use site.
Copy link

codecov bot commented Feb 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.20%. Comparing base (c9cdee3) to head (ee3619b).
Report is 14 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #186      +/-   ##
==========================================
- Coverage   96.28%   96.20%   -0.09%     
==========================================
  Files          36       36              
  Lines        1722     1790      +68     
==========================================
+ Hits         1658     1722      +64     
- Misses         64       68       +4     

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

@plietar
Copy link
Member Author

plietar commented Feb 21, 2024

Here are some benchmarks of the old version ("R6"), the new one ("list"), and the equivalent code without any wrappers, calling the C++ functions from R directly ("ptr"):
part1.pdf

https://gist.github.com/plietar/9d07ffb01b9ea7a27a12696b09242435

These compare the performance of doing Bitset$create(...), calling get_index_of(...) on a CategoryVariable and finally calling not().

The create and get_index_of results are a bit all over the place for larger bitsets, and I'm not sure why. The order in which I run the benchmarks makes a difference, the first one generally being the fastest. This is the same data as the previous graphs, but plotting each run over time: part2.pdf
Looking at profiles all the time is spent in memcpy/memset, so I would wager it is just some weird memory performance behaviour.

@plietar plietar changed the base branch from master to dev February 22, 2024 18:15
@plietar
Copy link
Member Author

plietar commented Feb 27, 2024

Unfortunately the generated documentation for this is pretty terrible. I need to figure out a good way of keeping the R6 look but with the alternative implementation.

@plietar
Copy link
Member Author

plietar commented Feb 27, 2024

Found a workaround for the documentation problem:

Instead of relying on roxygen to do the formatting, we include Rd markup directly in our code.
However we can make it a bit more convenient by using https://roxygen2.r-lib.org/articles/reuse.html#inline-code
We just have one function that outputs the Rd markdown and gets called in the comment block of every method.

It's not the prettiest implementation, but at least the output looks okay.

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.

Very hacky, I respect it 😆

Since this is localised to Bitsets, this seems a fine practical decision. But if we ever reuse this for other R6 objects, we should consider extending ROxygen to make documentation cleaner.

@plietar plietar merged commit e85b87e into mrc-ide:dev Feb 28, 2024
8 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.

2 participants