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

Experiment: Replace global mutable variables for random number generators #1

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

carsten-wenderdel
Copy link
Owner

@carsten-wenderdel carsten-wenderdel commented Feb 19, 2024

Summary

This is an experiment how a different usage of random number generators could improve CPU performance. The result is that simple_multi_job_100_benchmark takes 15% less time on an Apple M1 machine. This PR is not meant to be merged, but just an experiment.

Motivation

To see potential performance issues, I profiled CPU usage of simple_multi_job_100_benchmark (The description in Criterion says "a problem with 50 multi jobs". This was done with the Mac application Instruments. Noticeable are calls to the random number generator which take a substantial percentage of the whole time.

trace-vrp

Analysis

REPEATABLE_RNG and RANDOMIZED_RNG are global / thread local mutable variables. They are used by a non mutable Arc<dyn Random>.
There are several performance costs connected to that, mainly:

  • Smart pointers and dynamic dispatch
  • Thread local variables, where initialisation has to be checked with every access.
  • Branching via if to change between repeatable and non-deterministic behavior.

Idea

Use a new struct PureRandom which encapsulated the random number generator and pass it directly without smart pointers to each function. This way Rust can use zero cost abstractions handling it. Also our functions become pure functions: If identically seeded RNGs are given as arguments, the return value will always be the same. This can also be helpful for deterministic unit and performance tests.

Implementation

Not every usage of Random was replaced, only that in LegSelection. So most of vrp still uses the previous implementation. Now LegSelection needs to be mutable, so how this is handled in some Contexts had to be changed too - non mutable references don't work anymore here.

When used together with Rayon, each iteration needs a different RNG, so we need to create all of them before iteration. Each new DefaultPureRandom instance is different from each other, still everything is deterministic.

Performance testing

In general_benchmarks I've changed the sample size from 15 to 100 to get more reliable results. The time estimate for simple_multi_job_100_benchmark changed from 2.3671 seconds to 1.9978 s. This is roughly 15% less time or from the perspective: Before the change roughly 18% more time was needed.

Issues / Todos:

  • Is LegSelection the right place to contain PureRandom? Maybe remove it from there so that Stochastic doesn't have any values. This way we could change the contexts to contain only non mutable references again. The PureRandom would then be passed to each function as a separate value - this would also work in other cases where randomness is needed but LegSelection is not used.
  • Think again how new random structs are initialized. There are three ways: 1. Pseudo random based on a global/system RGN, 2. deterministic with a given seed, 3. a copy of an existing rng which then would return the same results. Which is best in which situation?
  • Do we need FakeRandom for unit tests? Maybe deterministic behaviour is enough, otherwise we might want to make some structs and functions generic instead of using dynamic dispatch.

@carsten-wenderdel carsten-wenderdel marked this pull request as draft February 19, 2024 08:43
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.

1 participant