Refactor: Update ServerTiming to use Arc and simplify header generation

This commit modifies the ServerTiming structure to utilize Arc for thread-safe reference counting, enhancing performance and memory management. The header generation logic is streamlined, and related code is updated to reflect these changes. Additionally, tests are adjusted to ensure proper functionality with the new implementation.

Co-authored-by: contact <[email protected]>

Author: lovasoa

src/render.rs

@@ -369,10 +369,8 @@ impl HeaderContext {
     }
 
     fn add_server_timing_header(&mut self) {
-        let header_value = self.request_context.server_timing.as_header_value();
-        if !header_value.is_empty() {
-            self.response
-                .insert_header(("Server-Timing", header_value.to_string()));
+        if let Some(header_value) = self.request_context.server_timing.header_value() {
+            self.response.insert_header(("Server-Timing", header_value));
         }
     }
 

src/webserver/database/execute_queries.rs

@@ -284,7 +284,7 @@ async fn take_connection<'a>(
     match db.connection.acquire().await {
         Ok(c) => {
             log::debug!("Acquired a database connection");
-            request.server_timing.borrow_mut().record("db_conn");
+            request.server_timing.record("db_conn");
             *conn = Some(c);
             Ok(conn.as_mut().unwrap())
         }

src/webserver/http.rs

@@ -47,7 +47,7 @@ pub struct RequestContext {
     pub is_embedded: bool,
     pub source_path: PathBuf,
     pub content_security_policy: ContentSecurityPolicy,
-    pub server_timing: ServerTiming,
+    pub server_timing: Arc<ServerTiming>,
 }
 
 async fn stream_response(stream: impl Stream<Item = DbItem>, mut renderer: AnyRenderBodyContext) {
@@ -176,23 +176,21 @@ async fn render_sql(
         .clone()
         .into_inner();
 
-    let mut req_param =
-        extract_request_info(srv_req, Arc::clone(&app_state), server_timing.clone())
-            .await
-            .map_err(|e| anyhow_err_to_actix(e, &app_state))?;
+    let mut req_param = extract_request_info(srv_req, Arc::clone(&app_state), server_timing)
+        .await
+        .map_err(|e| anyhow_err_to_actix(e, &app_state))?;
     log::debug!("Received a request with the following parameters: {req_param:?}");
 
-    req_param.server_timing.borrow_mut().record("parse_req");
+    req_param.server_timing.record("parse_req");
 
     let (resp_send, resp_recv) = tokio::sync::oneshot::channel::<HttpResponse>();
     let source_path: PathBuf = sql_file.source_path.clone();
     actix_web::rt::spawn(async move {
-        let server_timing_for_context = req_param.server_timing.borrow().clone();
         let request_context = RequestContext {
             is_embedded: req_param.get_variables.contains_key("_sqlpage_embed"),
             source_path,
             content_security_policy: ContentSecurityPolicy::with_random_nonce(),
-            server_timing: server_timing_for_context,
+            server_timing: Arc::clone(&req_param.server_timing),
         };
         let mut conn = None;
         let database_entries_stream =
@@ -286,8 +284,7 @@ async fn process_sql_request(
     sql_path: PathBuf,
 ) -> actix_web::Result<HttpResponse> {
     let app_state: &web::Data<AppState> = req.app_data().expect("app_state");
-    let mut server_timing = ServerTiming::new(!app_state.config.environment.is_prod());
-    server_timing.record("request");
+    let server_timing = ServerTiming::for_env(app_state.config.environment);
 
     let sql_file = app_state
         .sql_file_cache

src/webserver/http_request_info.rs

@@ -1,3 +1,4 @@
+use crate::webserver::server_timing::ServerTiming;
 use crate::AppState;
 use actix_multipart::form::bytes::Bytes;
 use actix_multipart::form::tempfile::TempFile;
@@ -42,7 +43,7 @@ pub struct RequestInfo {
     pub clone_depth: u8,
     pub raw_body: Option<Vec<u8>>,
     pub oidc_claims: Option<OidcClaims>,
-    pub server_timing: std::cell::RefCell<super::server_timing::ServerTiming>,
+    pub server_timing: Arc<super::server_timing::ServerTiming>,
 }
 
 impl RequestInfo {
@@ -63,7 +64,7 @@ impl RequestInfo {
             clone_depth: self.clone_depth + 1,
             raw_body: self.raw_body.clone(),
             oidc_claims: self.oidc_claims.clone(),
-            server_timing: std::cell::RefCell::new(self.server_timing.borrow().clone()),
+            server_timing: Arc::clone(&self.server_timing),
         }
     }
 }
@@ -80,7 +81,7 @@ impl Clone for RequestInfo {
 pub(crate) async fn extract_request_info(
     req: &mut ServiceRequest,
     app_state: Arc<AppState>,
-    server_timing: super::server_timing::ServerTiming,
+    server_timing: ServerTiming,
 ) -> anyhow::Result<RequestInfo> {
     let (http_req, payload) = req.parts_mut();
     let method = http_req.method().clone();
@@ -126,7 +127,7 @@ pub(crate) async fn extract_request_info(
         clone_depth: 0,
         raw_body,
         oidc_claims,
-        server_timing: std::cell::RefCell::new(server_timing),
+        server_timing: Arc::new(server_timing),
     })
 }
 
@@ -279,7 +280,7 @@ async fn is_file_field_empty(
 mod test {
     use super::super::http::SingleOrVec;
     use super::*;
-    use crate::app_config::AppConfig;
+    use crate::{app_config::AppConfig, webserver::server_timing::ServerTiming};
     use actix_web::{http::header::ContentType, test::TestRequest};
 
     #[actix_web::test]
@@ -288,7 +289,7 @@ mod test {
             serde_json::from_str::<AppConfig>(r#"{"listen_on": "localhost:1234"}"#).unwrap();
         let mut service_request = TestRequest::default().to_srv_request();
         let app_data = Arc::new(AppState::init(&config).await.unwrap());
-        let server_timing = super::server_timing::ServerTiming::new(false);
+        let server_timing = ServerTiming::default();
         let request_info = extract_request_info(&mut service_request, app_data, server_timing)
             .await
             .unwrap();
@@ -307,7 +308,7 @@ mod test {
             .set_payload("my_array[]=3&my_array[]=Hello%20World&repeated=1&repeated=2")
             .to_srv_request();
         let app_data = Arc::new(AppState::init(&config).await.unwrap());
-        let server_timing = super::server_timing::ServerTiming::new(false);
+        let server_timing = ServerTiming::default();
         let request_info = extract_request_info(&mut service_request, app_data, server_timing)
             .await
             .unwrap();
@@ -357,7 +358,7 @@ mod test {
             )
             .to_srv_request();
         let app_data = Arc::new(AppState::init(&config).await.unwrap());
-        let server_timing = super::server_timing::ServerTiming::new(false);
+        let server_timing = ServerTiming::enabled(false);
         let request_info = extract_request_info(&mut service_request, app_data, server_timing)
             .await
             .unwrap();

src/webserver/server_timing.rs

@@ -1,54 +1,70 @@
 use std::fmt::Write;
+use std::sync::Mutex;
 use std::time::Instant;
 
-#[derive(Debug, Clone)]
+use crate::app_config::DevOrProd;
+
+#[derive(Debug)]
 pub struct ServerTiming {
     enabled: bool,
-    previous_event: Instant,
-    header: String,
+    created_at: Instant,
+    events: Mutex<Vec<PerfEvent>>,
 }
 
-impl ServerTiming {
-    #[must_use]
-    pub fn new(enabled: bool) -> Self {
+#[derive(Debug)]
+struct PerfEvent {
+    time: Instant,
+    name: &'static str,
+}
+
+impl Default for ServerTiming {
+    fn default() -> Self {
         Self {
-            enabled,
-            previous_event: Instant::now(),
-            header: String::new(),
+            enabled: false,
+            created_at: Instant::now(),
+            events: Mutex::new(Vec::new()),
         }
     }
+}
 
-    pub fn record(&mut self, name: &'static str) {
-        if !self.enabled {
-            return;
-        }
-        let now = Instant::now();
-        let duration_ms = (now - self.previous_event).as_secs_f64() * 1000.0;
-        self.previous_event = now;
-
-        if !self.header.is_empty() {
-            self.header.push_str(", ");
+impl ServerTiming {
+    #[must_use]
+    pub fn enabled(enabled: bool) -> Self {
+        Self {
+            enabled,
+            ..Default::default()
         }
-        write!(&mut self.header, "{name};dur={duration_ms:.2}").unwrap();
     }
 
     #[must_use]
-    pub fn is_enabled(&self) -> bool {
-        self.enabled
+    pub fn for_env(env: DevOrProd) -> Self {
+        Self::enabled(!env.is_prod())
     }
 
-    #[must_use]
-    pub fn as_header_value(&self) -> &str {
+    pub fn record(&self, name: &'static str) {
         if self.enabled {
-            &self.header
-        } else {
-            ""
+            self.events.lock().unwrap().push(PerfEvent {
+                time: Instant::now(),
+                name,
+            });
         }
     }
-}
 
-impl Default for ServerTiming {
-    fn default() -> Self {
-        Self::new(false)
+    pub fn header_value(&self) -> Option<String> {
+        if !self.enabled {
+            return None;
+        }
+        let evts = self.events.lock().unwrap();
+        let mut s = String::with_capacity(evts.len() * 16);
+        let mut last = self.created_at;
+        for (i, PerfEvent { name, time }) in evts.iter().enumerate() {
+            if i > 0 {
+                s.push_str(", ");
+            }
+            let millis = time.saturating_duration_since(last).as_millis();
+            write!(&mut s, "{name};dur={millis}").ok()?;
+            last = *time;
+        }
+        Some(s)
     }
 }

tests/server_timing/mod.rs

@@ -1,4 +1,4 @@
-use actix_web::{http::StatusCode, test};
+use actix_web::http::StatusCode;
 use sqlpage::webserver::http::main_handler;
 
 use crate::common::{get_request_to, make_app_data_from_config, test_config};
@@ -32,7 +32,7 @@ async fn test_server_timing_enabled_in_development() -> actix_web::Result<()> {
     let app_data = make_app_data_from_config(config).await;
 
     let req = crate::common::get_request_to_with_data(
-        "/tests/sql_test_files/it_works_simple.sql",
+        "/tests/sql_test_files/it_works_sqrt.sql",
         app_data,
     )
     .await?
@@ -46,10 +46,6 @@ async fn test_server_timing_enabled_in_development() -> actix_web::Result<()> {
         .expect("Server-Timing header should be present in development mode");
     let header_value = server_timing_header.to_str().unwrap();
 
-    assert!(
-        header_value.contains("request;dur="),
-        "Should contain request timing: {header_value}"
-    );
     assert!(
         header_value.contains("sql_file;dur="),
         "Should contain sql_file timing: {header_value}"
@@ -72,7 +68,7 @@ async fn test_server_timing_enabled_in_development() -> actix_web::Result<()> {
 
 #[actix_web::test]
 async fn test_server_timing_format() -> actix_web::Result<()> {
-    let req = get_request_to("/tests/sql_test_files/it_works_simple.sql")
+    let req = get_request_to("/tests/sql_test_files/it_works_sqrt.sql")
         .await?
         .to_srv_request();
     let resp = main_handler(req).await?;