perf: Make every gossip thread use its own randomness instance, reducing contention by ValarDragon · Pull Request #3006 · cometbft/cometbft

ValarDragon · 2024-05-05T23:47:13Z

As noted in that issue, we currently are doing extra CPU overhead and mutex contention for getting a random number. This PR removes this overhead by making every performance sensitive thread have its own rand instance.

In a subsequent PR, we can cleanup all the testing usages, and likely just entirely delete our internal randomness package. I didn't do that in this PR to keep it straightforward to verify.

PR checklist

Tests written/updated
Changelog entry added in .changelog (we use unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments
Title follows the Conventional Commits spec

…ntention

ValarDragon · 2024-05-05T23:54:22Z

internal/rand/random.go

 // NOTE: This relies on the os's random number generator.
 // For real security, we should salt that with some seed.
 // See github.com/cometbft/cometbft/crypto for a more secure reader.


I disagree with this comment, but will leave to the general cleanup PR for this package to remove.

This comment does not make sense given the preamble of this source file...

melekes

Thanks @ValarDragon ❤️

internal/consensus/reactor.go

ValarDragon · 2024-05-06T07:55:41Z

internal/rand/random.go

+	// G404: Use of weak random number generator (math/rand instead of crypto/rand)
+	//nolint:gosec


Note that the way we create the local Rand object also has this nolint, so its not newly introduced.

cason

Using a synchronized random source here does not make sense.

The solution, however, is pretty complex, but it is what we can do right now. I would also check other internal uses of this internal random library to understand when we actually need a thread-safe version of it.

cason · 2024-05-07T16:27:53Z

internal/rand/random_test.go

 	wg.Wait()
 }

+// Makes a new stdlib random instance 100 times concurrently.


Should we test that cRandBytes is thread safe instead?

cason · 2024-05-07T16:29:01Z

internal/rand/random.go

 // NOTE: This relies on the os's random number generator.
 // For real security, we should salt that with some seed.
 // See github.com/cometbft/cometbft/crypto for a more secure reader.


This comment does not make sense given the preamble of this source file...

cason · 2024-05-07T16:36:55Z

internal/consensus/reactor.go

Somehow impressed that the bit array method is only used here. I wonder whether implementing part of this logic here, instead of there, could be a better solution.

At the end, we are counting the 1 bits, then getting a random index from zero to this number, then retrieving the i-th 1 bit. The random part we could implement outside the bit array library.

ValarDragon · 2024-05-07T17:23:47Z

Every other usage from what I can tell can just be replaced with math.Rand, but wanted to do that in a separate PR. (Math.rand's global instance is thread safe, by internally doing CAS operations and a retry loop with no wait time)

cason · 2024-05-08T15:33:17Z

The problem of math/rand is the setup/seed used. Paying the synchronization costs does not make much sense in most of our use cases.

…ing contention (#3006) Closes #3005 As noted in that issue, we currently are doing extra CPU overhead and mutex contention for getting a random number. This PR removes this overhead by making every performance sensitive thread have its own rand instance. In a subsequent PR, we can cleanup all the testing usages, and likely just entirely delete our internal randomness package. I didn't do that in this PR to keep it straightforward to verify. --- #### PR checklist - [x] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec (cherry picked from commit df28e73) # Conflicts: # internal/consensus/reactor.go

…ing contention (backport #3006) (#3045) Closes #3005 As noted in that issue, we currently are doing extra CPU overhead and mutex contention for getting a random number. This PR removes this overhead by making every performance sensitive thread have its own rand instance. In a subsequent PR, we can cleanup all the testing usages, and likely just entirely delete our internal randomness package. I didn't do that in this PR to keep it straightforward to verify. --- #### PR checklist - [x] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request #3006 done by [Mergify](https://mergify.com). --------- Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

…ing contention (cometbft#3006) Closes cometbft#3005 As noted in that issue, we currently are doing extra CPU overhead and mutex contention for getting a random number. This PR removes this overhead by making every performance sensitive thread have its own rand instance. In a subsequent PR, we can cleanup all the testing usages, and likely just entirely delete our internal randomness package. I didn't do that in this PR to keep it straightforward to verify. --- - [x] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

…ing contention (cometbft#3006) Closes cometbft#3005 As noted in that issue, we currently are doing extra CPU overhead and mutex contention for getting a random number. This PR removes this overhead by making every performance sensitive thread have its own rand instance. In a subsequent PR, we can cleanup all the testing usages, and likely just entirely delete our internal randomness package. I didn't do that in this PR to keep it straightforward to verify. --- - [x] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec (cherry picked from commit f55b9f4)

…… (backport #77) (#82) * perf: Make every gossip thread use its own randomness instance, reducing contention (cometbft#3006) Closes cometbft#3005 As noted in that issue, we currently are doing extra CPU overhead and mutex contention for getting a random number. This PR removes this overhead by making every performance sensitive thread have its own rand instance. In a subsequent PR, we can cleanup all the testing usages, and likely just entirely delete our internal randomness package. I didn't do that in this PR to keep it straightforward to verify. --- - [x] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec (cherry picked from commit f55b9f4) * Add Changelgo (cherry picked from commit ce04f04) # Conflicts: # CHANGELOG.md * Fix changelog further (cherry picked from commit bd34ce6) # Conflicts: # CHANGELOG.md --------- Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Dev Ojha <dojha@berkeley.edu> Co-authored-by: PaddyMc <paddymchale@hotmail.com>

…… (backport #77) (#82) * perf: Make every gossip thread use its own randomness instance, reducing contention (cometbft#3006) Closes cometbft#3005 As noted in that issue, we currently are doing extra CPU overhead and mutex contention for getting a random number. This PR removes this overhead by making every performance sensitive thread have its own rand instance. In a subsequent PR, we can cleanup all the testing usages, and likely just entirely delete our internal randomness package. I didn't do that in this PR to keep it straightforward to verify. --- - [x] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec (cherry picked from commit f55b9f4) * Add Changelgo (cherry picked from commit ce04f04) * Fix changelog further (cherry picked from commit bd34ce6) --------- Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Dev Ojha <dojha@berkeley.edu> Co-authored-by: PaddyMc <paddymchale@hotmail.com>

…… (backport #77) (#82) (#132) * perf: Make every gossip thread use its own randomness instance, reducing contention (cometbft#3006) Closes cometbft#3005 As noted in that issue, we currently are doing extra CPU overhead and mutex contention for getting a random number. This PR removes this overhead by making every performance sensitive thread have its own rand instance. In a subsequent PR, we can cleanup all the testing usages, and likely just entirely delete our internal randomness package. I didn't do that in this PR to keep it straightforward to verify. --- - [x] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec (cherry picked from commit f55b9f4) * Add Changelgo (cherry picked from commit ce04f04) * Fix changelog further (cherry picked from commit bd34ce6) --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Dev Ojha <dojha@berkeley.edu>

Make every gossip thread use its own randomness instance, reducing co…

cf31f70

…ntention

ValarDragon requested a review from a team as a code owner May 5, 2024 23:47

ValarDragon requested a review from a team May 5, 2024 23:47

Add comments

d7b64b7

ValarDragon changed the title ~~Make every gossip thread use its own randomness instance, reducing contention~~ perf: Make every gossip thread use its own randomness instance, reducing contention May 5, 2024

ValarDragon commented May 5, 2024

View reviewed changes

ValarDragon added 3 commits May 5, 2024 16:56

Add changelog

7860711

Actually add changelog this time

0efc53c

Fix lint

af62d88

melekes approved these changes May 6, 2024

View reviewed changes

internal/consensus/reactor.go Outdated Show resolved Hide resolved

Fix lint, rename r -> rng

2a97968

ValarDragon commented May 6, 2024

View reviewed changes

cason approved these changes May 7, 2024

View reviewed changes

melekes added the backport-to-v1.x label May 9, 2024

melekes added this pull request to the merge queue May 9, 2024

Merged via the queue into cometbft:main with commit df28e73 May 9, 2024

mergify bot mentioned this pull request May 9, 2024

perf: Make every gossip thread use its own randomness instance, reducing contention (backport #3006) #3045

Merged

4 tasks

ValarDragon mentioned this pull request May 26, 2024

perf: Make every gossip thread use its own randomness instance, reduc… osmosis-labs/cometbft#77

Merged

7 tasks

mergify bot mentioned this pull request May 28, 2024

perf: Make every gossip thread use its own randomness instance, reduc… (backport #77) osmosis-labs/cometbft#82

Merged

7 tasks

PaddyMc mentioned this pull request Aug 19, 2024

perf: Make every gossip thread use its own randomness instance, reduc… osmosis-labs/cometbft#132

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: Make every gossip thread use its own randomness instance, reducing contention#3006

perf: Make every gossip thread use its own randomness instance, reducing contention#3006
melekes merged 6 commits intocometbft:mainfrom
osmosis-labs:dev/use_per_gossiproutine_rand

ValarDragon commented May 5, 2024 •

edited

Loading

Uh oh!

ValarDragon May 5, 2024

Uh oh!

cason May 7, 2024

Uh oh!

melekes left a comment

Uh oh!

Uh oh!

ValarDragon May 6, 2024

Uh oh!

cason left a comment

Uh oh!

cason May 7, 2024

Uh oh!

cason May 7, 2024

Uh oh!

cason May 7, 2024

Uh oh!

ValarDragon commented May 7, 2024 •

edited

Loading

Uh oh!

cason commented May 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		// G404: Use of weak random number generator (math/rand instead of crypto/rand)
		//nolint:gosec

Conversation

ValarDragon commented May 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

ValarDragon May 5, 2024

Choose a reason for hiding this comment

Uh oh!

cason May 7, 2024

Choose a reason for hiding this comment

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ValarDragon May 6, 2024

Choose a reason for hiding this comment

Uh oh!

cason left a comment

Choose a reason for hiding this comment

Uh oh!

cason May 7, 2024

Choose a reason for hiding this comment

Uh oh!

cason May 7, 2024

Choose a reason for hiding this comment

Uh oh!

cason May 7, 2024

Choose a reason for hiding this comment

Uh oh!

ValarDragon commented May 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cason commented May 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ValarDragon commented May 5, 2024 •

edited

Loading

ValarDragon commented May 7, 2024 •

edited

Loading