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

Change flowrate.clock() to be based on a variable that is scheduled to update every 20ms #2958

Closed
Tracked by #3053
ValarDragon opened this issue May 1, 2024 · 2 comments · Fixed by #3016 or osmosis-labs/cometbft#88
Labels
enhancement New feature or request needs-triage This issue/PR has not yet been triaged by the team.

Comments

@ValarDragon
Copy link
Collaborator

Feature Request

Summary

Currently the flowrate package has every single call to it do a time.Now() call, and then round it to the nearest 20ms, and uses that as a "flowrate epoch" internally for its computations.

This leads to lots of time spent in time.Now() and rounding logic. Result from a 1 hour live osmosis node profile (~1 month ago), that overlapped with the epoch (60s long):
image

10 seconds spent in time.Now() checking, when were trying to just send the packet, feels very significant. Unfortunately I don't have statistics for the number of packets we sent in this time period to be able to bound it though. Assuming the current flowrate functionality is what we want, we actually have a much more efficient way to implement this function. We can have a global "currentClock" var. Every clock() function is an atomic read of this var. We spin up another goroutine at initialization that atomically writes this var every 20ms. This should change this clock time to then be a tiny fraction of what it is now, with very little consensus delay.

Right now the code is currently

// czero is the process start time rounded down to the nearest clockRate
// increment.
var czero = time.Now().Round(clockRate)

// clock returns a low resolution timestamp relative to the process start time.
func clock() time.Duration {
	return time.Now().Round(clockRate).Sub(czero)
}

so this would change to roughly

var curClockTime time.Duration = time.Duration(0)

func clockTimeUpdateLoop() {
   for {
          time.Sleep(clockRate)
          newClockTime = oldclockfn()
          Atomic.SetInt64(newClockTime, &curClockTime)
    }
}

// clock returns a low resolution timestamp relative to the process start time.
func clock() time.Duration {
	return time.Duration(Atomic.LoadInt64(curClockTime))
}

I don't think we care about high fidelity time keeping of the flow rate, but if we did, then we could do the epoching goroutine do a time.Sleep for say 19.9ms, and check for the final boundary point with a .1ms sleep loop.

Problem Definition

Lowers CPU contention in trying to send packets.

@ValarDragon ValarDragon added enhancement New feature or request needs-triage This issue/PR has not yet been triaged by the team. labels May 1, 2024
@ValarDragon
Copy link
Collaborator Author

Happy to try PR'ing a change if this sounds acceptable to folks. (Having a single clock variable, and a background process started on init for updating it. Alternatively could make the background process start on the first Flowrate.New)

@sergio-mena
Copy link
Contributor

Sounds reasonable to me. I'd prefer the new goroutine to start upon first use of flowrate

github-merge-queue bot pushed a commit that referenced this issue May 27, 2024
…3016)

Closes #2958 
Speeds up flowrate by removing time.Now() calls from every `clock` call,
instead making one goroutine responsible for all clock reads. On my most
recent mainnet benchmark, this should shave 14% off of the synchronous
time in sending packets:


![image](https://github.com/cometbft/cometbft/assets/6440154/f2f65080-7bf1-4c4c-9775-774d97ed016a)

There may be slight differences at the boundary of these 20ms windows,
but I do not think that is important.

---

#### PR checklist

- [x] Tests written/updated - existing tests pass
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
ValarDragon added a commit to osmosis-labs/cometbft that referenced this issue May 31, 2024
…ometbft#3016)

Closes cometbft#2958
Speeds up flowrate by removing time.Now() calls from every `clock` call,
instead making one goroutine responsible for all clock reads. On my most
recent mainnet benchmark, this should shave 14% off of the synchronous
time in sending packets:

![image](https://github.com/cometbft/cometbft/assets/6440154/f2f65080-7bf1-4c4c-9775-774d97ed016a)

There may be slight differences at the boundary of these 20ms windows,
but I do not think that is important.

---

- [x] Tests written/updated - existing tests pass
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
mergify bot pushed a commit to osmosis-labs/cometbft that referenced this issue May 31, 2024
…ometbft#3016)

Closes cometbft#2958
Speeds up flowrate by removing time.Now() calls from every `clock` call,
instead making one goroutine responsible for all clock reads. On my most
recent mainnet benchmark, this should shave 14% off of the synchronous
time in sending packets:

![image](https://github.com/cometbft/cometbft/assets/6440154/f2f65080-7bf1-4c4c-9775-774d97ed016a)

There may be slight differences at the boundary of these 20ms windows,
but I do not think that is important.

---

- [x] Tests written/updated - existing tests pass
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

(cherry picked from commit 8d51a10)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs-triage This issue/PR has not yet been triaged by the team.
Projects
None yet
2 participants