Perf improvements #166
Perf improvements #166
Conversation
Ran some tests sending as many packets to the proxy and made some changes in the process. - Limit the max number of packets we're processing at a time. - Make session to not expose any mutable APIs. This allows us to not have to avoid locking the session when working with it. A thread only acquires a lock in order to put a packet on the socket and it immediately releases it after that. - Use an atomic integer to track session expiration rather than an explicit lock. - Enable parking_lot mutex internally within Tokio. Tokio's lock is really slow and is currently one bottleneck in the code. It has the option to use parking_lot internally which helped speed things up a bit. Ideally, we won't have any tokio locks on the hot path when processing packets - currently the session map uses this because that lock is held across await boundaries - will look into whether we can avoid this later on. This change also fixes a couple race conditions - In ensure_session, between grabbing a read and write lock, another thread could create a session, leading to multiple sessions being created. - A similar bug in prune_sessions - if a session receives a packet between listing `remove_keys` and actually removing sessions, we will incorrectly remove that session
There was a problem hiding this comment.
Lots of fun stuff! Nice!
I mentioned in the comment - can you share how you performed the performance tests (also how you tracked the metrics)? Maybe this is the start of some performance suites?
If you can't, can you paste at least some of the results in here? Would be good to have even just a light history of performance metrics on the project that we can look back on over time, even if it's just for a baseline.
| Ok(_) => { | ||
| session.increment_expiration().await; | ||
| if let Some(response) = result { | ||
| for endpoint in response.endpoints.iter() { |
There was a problem hiding this comment.
Can we pull this out into another function? Even if we pulled out the block at 274 (inside for endpoint in response.endpoints.iter() {) into it's own function - session_send_packet (as a thought on a name?) - I think it would be easier to follow, as the complicated locking logic would be wrapped into it's own area.
Feels to me like this code block is a bit too large inside this tokio::spawn to otherwise track what is happening.
There was a problem hiding this comment.
Extracted to session_send_packet functions!
src/proxy/sessions/session.rs
Outdated
| pub async fn increment_expiration(&mut self) { | ||
| let expiration = self.expiration.clone(); | ||
| Session::inc_expiration(expiration).await | ||
| /// increment_expiration set the increments the expiration value by the session timeout |
There was a problem hiding this comment.
| /// increment_expiration set the increments the expiration value by the session timeout | |
| /// update_expiration increments the expiration value by the session timeout |
src/proxy/sessions/session.rs
Outdated
| Session::inc_expiration(expiration).await | ||
| /// increment_expiration set the increments the expiration value by the session timeout | ||
| pub fn update_expiration(&self, ttl: Duration) -> Result<()> { | ||
| Self::do_update_expiration(&self.expiration, ttl) | ||
| } | ||
|
|
||
| /// increment_expiration increments the expiration value by the session timeout (internal) |
There was a problem hiding this comment.
| /// increment_expiration increments the expiration value by the session timeout (internal) | |
| /// do_update_expiration increments the expiration value by the session timeout (internal) |
src/proxy/sessions/session.rs
Outdated
| } | ||
|
|
||
| /// Sends a packet to the Session's dest. | ||
| pub async fn send_to(&mut self, buf: &[u8]) -> Result<Option<usize>> { | ||
| pub async fn send_to(&self, buf: &[u8]) -> Result<Option<usize>> { |
There was a problem hiding this comment.
Since we're in here, should this be "send" rather than "send_to" ?
markmandel
added
the
area/performance
Ran some tests sending as many packets to the proxy and made some changes in the process.
Limit the max number of packets we're processing at a time.
Make session to not expose any mutable APIs. This allows us to not have to avoid locking the
session when working with it. A thread only acquires a lock in order to put a packet on the
socket and it immediately releases it after that.
Use an atomic integer to track session expiration rather than an explicit lock.
Enable parking_lot mutex internally within Tokio. Tokio's lock is really slow and
is currently one bottleneck in the code. It has the option to use parking_lot internally
which helped speed things up a bit.
Ideally, we won't have any tokio locks on the hot path when processing
packets - currently the session map uses this because that lock is held across await
boundaries - will look into whether we can avoid this later on.
This change also fixes a couple race conditions
In ensure_session, between grabbing a read and write lock, another thread could create
a session, leading to multiple sessions being created.
A similar bug in prune_sessions - if a session receives a packet between listing
remove_keysand actually removing sessions, we will incorrectly remove that session