From d10ad4465f3c77c05ce40885904b71dcde6fd762 Mon Sep 17 00:00:00 2001 From: Connor Imes Date: Tue, 7 Feb 2017 12:45:11 -0600 Subject: [PATCH 1/4] Update heartbeats-simple to 0.4; Include recently added profiler categories in heartbeat logging --- components/profile/heartbeats.rs | 3 +++ tests/heartbeats/characterize.py | 3 +++ 2 files changed, 6 insertions(+) diff --git a/components/profile/heartbeats.rs b/components/profile/heartbeats.rs index b05cbf74b71..761a1cf4d54 100644 --- a/components/profile/heartbeats.rs +++ b/components/profile/heartbeats.rs @@ -65,6 +65,9 @@ pub fn init() { maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptWorkerEvent); maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptServiceWorkerEvent); maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptParseXML); + maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptEnterFullscreen); + maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptExitFullscreen); + maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptWebVREvent); maybe_create_heartbeat(&mut hbs, ProfilerCategory::ApplicationHeartbeat); unsafe { HBS = Some(mem::transmute(Box::new(hbs))); diff --git a/tests/heartbeats/characterize.py b/tests/heartbeats/characterize.py index 948f5b9de43..9674119c44b 100644 --- a/tests/heartbeats/characterize.py +++ b/tests/heartbeats/characterize.py @@ -65,6 +65,9 @@ HEARTBEAT_PROFILER_CATEGORIES = [ ("ScriptWorkerEvent", HEARTBEAT_DEFAULT_WINDOW_SIZE), ("ScriptServiceWorkerEvent", HEARTBEAT_DEFAULT_WINDOW_SIZE), ("ScriptParseXML", HEARTBEAT_DEFAULT_WINDOW_SIZE), + ("ScriptEnterFullscreen", HEARTBEAT_DEFAULT_WINDOW_SIZE), + ("ScriptExitFullscreen", HEARTBEAT_DEFAULT_WINDOW_SIZE), + ("ScriptWebVREvent", HEARTBEAT_DEFAULT_WINDOW_SIZE), ("ApplicationHeartbeat", 100), ] ENERGY_READER_BIN = "energymon-file-provider" From e602162a3d8af7aa8fff4fde63a1570838aab368 Mon Sep 17 00:00:00 2001 From: Connor Imes Date: Tue, 7 Feb 2017 14:20:43 -0600 Subject: [PATCH 2/4] Create heartbeats dynamically as needed - avoids the need for hardcoding and maintaining heartbeats for profiler categories. Use consistent variable names. Use Box::into_raw and Box::from_raw instead of std::mem::transmute. --- components/profile/heartbeats.rs | 74 ++++++-------------------------- 1 file changed, 14 insertions(+), 60 deletions(-) diff --git a/components/profile/heartbeats.rs b/components/profile/heartbeats.rs index 761a1cf4d54..0919ccb84e7 100644 --- a/components/profile/heartbeats.rs +++ b/components/profile/heartbeats.rs @@ -11,7 +11,6 @@ use std::collections::HashMap; use std::env::var_os; use std::error::Error; use std::fs::File; -use std::mem; use std::path::Path; @@ -19,71 +18,23 @@ static mut HBS: Option<*mut HashMap> = None; /// Initialize heartbeats pub fn init() { - let mut hbs: HashMap = HashMap::new(); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::Compositing); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::LayoutPerform); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::LayoutStyleRecalc); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::LayoutTextShaping); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::LayoutRestyleDamagePropagation); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::LayoutNonIncrementalReset); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::LayoutSelectorMatch); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::LayoutTreeBuilder); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::LayoutDamagePropagate); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::LayoutGeneratedContent); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::LayoutDisplayListSorting); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::LayoutFloatPlacementSpeculation); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::LayoutMain); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::LayoutStoreOverflow); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::LayoutParallelWarmup); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::LayoutDispListBuild); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::NetHTTPRequestResponse); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::PaintingPerTile); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::PaintingPrepBuff); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::Painting); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ImageDecoding); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ImageSaving); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptAttachLayout); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptConstellationMsg); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptDevtoolsMsg); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptDocumentEvent); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptDomEvent); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptEvaluate); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptEvent); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptFileRead); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptImageCacheMsg); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptInputEvent); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptNetworkEvent); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptParseHTML); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptPlannedNavigation); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptResize); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptSetScrollState); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptSetViewport); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptTimerEvent); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptStylesheetLoad); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptUpdateReplacedElement); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptWebSocketEvent); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptWorkerEvent); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptServiceWorkerEvent); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptParseXML); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptEnterFullscreen); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptExitFullscreen); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ScriptWebVREvent); + let mut hbs: Box> = Box::new(HashMap::new()); maybe_create_heartbeat(&mut hbs, ProfilerCategory::ApplicationHeartbeat); unsafe { - HBS = Some(mem::transmute(Box::new(hbs))); + HBS = Some(Box::into_raw(hbs)); } } /// Log regmaining buffer data and cleanup heartbeats pub fn cleanup() { unsafe { - if let Some(hbs) = HBS { - let mut h: Box> = mem::transmute(hbs); - for (_, mut v) in h.iter_mut() { + if let Some(hbs_ptr) = HBS { + let mut hbs: Box> = Box::from_raw(hbs_ptr); + for (_, mut v) in hbs.iter_mut() { // log any remaining heartbeat records before dropping log_heartbeat_records(v); } - h.clear(); + hbs.clear(); } HBS = None; } @@ -92,7 +43,7 @@ pub fn cleanup() { /// Check if a heartbeat exists for the given category pub fn is_heartbeat_enabled(category: &ProfilerCategory) -> bool { unsafe { - HBS.map_or(false, |m| (*m).contains_key(category)) + HBS.map_or(false, |hbs_ptr| (*hbs_ptr).contains_key(category) || is_create_heartbeat(category)) } } @@ -103,8 +54,11 @@ pub fn maybe_heartbeat(category: &ProfilerCategory, start_energy: u64, end_energy: u64) { unsafe { - if let Some(map) = HBS { - if let Some(mut h) = (*map).get_mut(category) { + if let Some(hbs_ptr) = HBS { + if !(*hbs_ptr).contains_key(category) { + maybe_create_heartbeat(&mut (*hbs_ptr), category.clone()); + } + if let Some(mut h) = (*hbs_ptr).get_mut(category) { (*h).heartbeat(0, 1, start_time, end_time, start_energy, end_energy); } } @@ -179,8 +133,8 @@ fn log_heartbeat_records(hb: &mut Heartbeat) { /// When this is called from native C, the heartbeat is safely locked extern fn heartbeat_window_callback(hb: *const HeartbeatContext) { unsafe { - if let Some(map) = HBS { - for (_, v) in (*map).iter_mut() { + if let Some(hbs_ptr) = HBS { + for (_, v) in (*hbs_ptr).iter_mut() { if &v.hb as *const HeartbeatContext == hb { log_heartbeat_records(v); } From a13b2d447bdf1e6df37dea90d6df18728f3dadf8 Mon Sep 17 00:00:00 2001 From: Connor Imes Date: Fri, 10 Feb 2017 11:17:18 -0600 Subject: [PATCH 3/4] Use spinlock to synchronize access to static mut with pointer. Fixes #15471 --- components/profile/heartbeats.rs | 77 ++++++++++++++++++++++---------- 1 file changed, 54 insertions(+), 23 deletions(-) diff --git a/components/profile/heartbeats.rs b/components/profile/heartbeats.rs index 0919ccb84e7..b3c199a8fd8 100644 --- a/components/profile/heartbeats.rs +++ b/components/profile/heartbeats.rs @@ -12,39 +12,67 @@ use std::env::var_os; use std::error::Error; use std::fs::File; use std::path::Path; +use std::sync::atomic::{ATOMIC_BOOL_INIT, AtomicBool, Ordering}; static mut HBS: Option<*mut HashMap> = None; +// unfortunately can't encompass the actual hashmap in a Mutex (Heartbeat isn't Send/Sync), so we'll use a spinlock +static mut HBS_SPINLOCK: AtomicBool = ATOMIC_BOOL_INIT; + +fn lock_and_work(work: F) -> R where F: FnOnce() -> R { + unsafe { + while HBS_SPINLOCK.compare_and_swap(false, true, Ordering::SeqCst) {} + } + let result = work(); + unsafe { + HBS_SPINLOCK.store(false, Ordering::SeqCst); + } + result +} + /// Initialize heartbeats pub fn init() { let mut hbs: Box> = Box::new(HashMap::new()); maybe_create_heartbeat(&mut hbs, ProfilerCategory::ApplicationHeartbeat); - unsafe { - HBS = Some(Box::into_raw(hbs)); - } + lock_and_work(|| + unsafe { + HBS = Some(Box::into_raw(hbs)); + } + ); } /// Log regmaining buffer data and cleanup heartbeats pub fn cleanup() { - unsafe { - if let Some(hbs_ptr) = HBS { - let mut hbs: Box> = Box::from_raw(hbs_ptr); - for (_, mut v) in hbs.iter_mut() { - // log any remaining heartbeat records before dropping - log_heartbeat_records(v); + let hbs_opt: Option>> = lock_and_work(|| + unsafe { + match HBS { + Some(hbs_ptr) => { + let ret = Some(Box::from_raw(hbs_ptr)); + HBS = None; + ret + }, + None => None, } - hbs.clear(); } - HBS = None; + ); + if let Some(mut hbs) = hbs_opt { + for (_, mut v) in hbs.iter_mut() { + // log any remaining heartbeat records before dropping + log_heartbeat_records(v); + } + hbs.clear(); } } /// Check if a heartbeat exists for the given category pub fn is_heartbeat_enabled(category: &ProfilerCategory) -> bool { - unsafe { - HBS.map_or(false, |hbs_ptr| (*hbs_ptr).contains_key(category) || is_create_heartbeat(category)) - } + let is_enabled = lock_and_work(|| + unsafe { + HBS.map_or(false, |hbs_ptr| (*hbs_ptr).contains_key(category)) + } + ); + is_enabled || is_create_heartbeat(category) } /// Issue a heartbeat (if one exists) for the given category @@ -53,16 +81,18 @@ pub fn maybe_heartbeat(category: &ProfilerCategory, end_time: u64, start_energy: u64, end_energy: u64) { - unsafe { - if let Some(hbs_ptr) = HBS { - if !(*hbs_ptr).contains_key(category) { - maybe_create_heartbeat(&mut (*hbs_ptr), category.clone()); - } - if let Some(mut h) = (*hbs_ptr).get_mut(category) { - (*h).heartbeat(0, 1, start_time, end_time, start_energy, end_energy); + lock_and_work(|| + unsafe { + if let Some(hbs_ptr) = HBS { + if !(*hbs_ptr).contains_key(category) { + maybe_create_heartbeat(&mut (*hbs_ptr), category.clone()); + } + if let Some(mut h) = (*hbs_ptr).get_mut(category) { + (*h).heartbeat(0, 1, start_time, end_time, start_energy, end_energy); + } } } - } + ); } // TODO(cimes): Android doesn't really do environment variables. Need a better way to configure dynamically. @@ -130,7 +160,8 @@ fn log_heartbeat_records(hb: &mut Heartbeat) { } /// Callback function used to log the window buffer. -/// When this is called from native C, the heartbeat is safely locked +/// When this is called from native C, the heartbeat is safely locked internally and the global lock is held. +/// If calling from this file, you must already hold the global lock! extern fn heartbeat_window_callback(hb: *const HeartbeatContext) { unsafe { if let Some(hbs_ptr) = HBS { From ab3a3b43c1e8ed67129b45a60d39667d215be92b Mon Sep 17 00:00:00 2001 From: Connor Imes Date: Tue, 14 Feb 2017 13:46:50 -0600 Subject: [PATCH 4/4] Handle synchronization in a private module with safer closure --- components/profile/heartbeats.rs | 106 ++++++++++++++++--------------- 1 file changed, 54 insertions(+), 52 deletions(-) diff --git a/components/profile/heartbeats.rs b/components/profile/heartbeats.rs index b3c199a8fd8..4661dad6183 100644 --- a/components/profile/heartbeats.rs +++ b/components/profile/heartbeats.rs @@ -2,61 +2,37 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - use heartbeats_simple::HeartbeatPow as Heartbeat; -use heartbeats_simple::HeartbeatPowContext as HeartbeatContext; use profile_traits::time::ProfilerCategory; +use self::synchronized_heartbeat::{heartbeat_window_callback, lock_and_work}; use servo_config::opts; use std::collections::HashMap; use std::env::var_os; use std::error::Error; use std::fs::File; use std::path::Path; -use std::sync::atomic::{ATOMIC_BOOL_INIT, AtomicBool, Ordering}; - - -static mut HBS: Option<*mut HashMap> = None; - -// unfortunately can't encompass the actual hashmap in a Mutex (Heartbeat isn't Send/Sync), so we'll use a spinlock -static mut HBS_SPINLOCK: AtomicBool = ATOMIC_BOOL_INIT; - -fn lock_and_work(work: F) -> R where F: FnOnce() -> R { - unsafe { - while HBS_SPINLOCK.compare_and_swap(false, true, Ordering::SeqCst) {} - } - let result = work(); - unsafe { - HBS_SPINLOCK.store(false, Ordering::SeqCst); - } - result -} /// Initialize heartbeats pub fn init() { - let mut hbs: Box> = Box::new(HashMap::new()); - maybe_create_heartbeat(&mut hbs, ProfilerCategory::ApplicationHeartbeat); - lock_and_work(|| - unsafe { - HBS = Some(Box::into_raw(hbs)); + lock_and_work(|hbs_opt| + if hbs_opt.is_none() { + let mut hbs: Box> = Box::new(HashMap::new()); + maybe_create_heartbeat(&mut hbs, ProfilerCategory::ApplicationHeartbeat); + *hbs_opt = Some(Box::into_raw(hbs)) } ); } /// Log regmaining buffer data and cleanup heartbeats pub fn cleanup() { - let hbs_opt: Option>> = lock_and_work(|| - unsafe { - match HBS { - Some(hbs_ptr) => { - let ret = Some(Box::from_raw(hbs_ptr)); - HBS = None; - ret - }, - None => None, + let hbs_opt_box: Option>> = lock_and_work(|hbs_opt| + hbs_opt.take().map(|hbs_ptr| + unsafe { + Box::from_raw(hbs_ptr) } - } + ) ); - if let Some(mut hbs) = hbs_opt { + if let Some(mut hbs) = hbs_opt_box { for (_, mut v) in hbs.iter_mut() { // log any remaining heartbeat records before dropping log_heartbeat_records(v); @@ -67,10 +43,12 @@ pub fn cleanup() { /// Check if a heartbeat exists for the given category pub fn is_heartbeat_enabled(category: &ProfilerCategory) -> bool { - let is_enabled = lock_and_work(|| - unsafe { - HBS.map_or(false, |hbs_ptr| (*hbs_ptr).contains_key(category)) - } + let is_enabled = lock_and_work(|hbs_opt| + hbs_opt.map_or(false, |hbs_ptr| + unsafe { + (*hbs_ptr).contains_key(category) + } + ) ); is_enabled || is_create_heartbeat(category) } @@ -81,9 +59,9 @@ pub fn maybe_heartbeat(category: &ProfilerCategory, end_time: u64, start_energy: u64, end_energy: u64) { - lock_and_work(|| - unsafe { - if let Some(hbs_ptr) = HBS { + lock_and_work(|hbs_opt| + if let Some(hbs_ptr) = *hbs_opt { + unsafe { if !(*hbs_ptr).contains_key(category) { maybe_create_heartbeat(&mut (*hbs_ptr), category.clone()); } @@ -159,15 +137,39 @@ fn log_heartbeat_records(hb: &mut Heartbeat) { } } -/// Callback function used to log the window buffer. -/// When this is called from native C, the heartbeat is safely locked internally and the global lock is held. -/// If calling from this file, you must already hold the global lock! -extern fn heartbeat_window_callback(hb: *const HeartbeatContext) { - unsafe { - if let Some(hbs_ptr) = HBS { - for (_, v) in (*hbs_ptr).iter_mut() { - if &v.hb as *const HeartbeatContext == hb { - log_heartbeat_records(v); +mod synchronized_heartbeat { + use heartbeats_simple::HeartbeatPow as Heartbeat; + use heartbeats_simple::HeartbeatPowContext as HeartbeatContext; + use profile_traits::time::ProfilerCategory; + use std::collections::HashMap; + use std::sync::atomic::{ATOMIC_BOOL_INIT, AtomicBool, Ordering}; + use super::log_heartbeat_records; + + static mut HBS: Option<*mut HashMap> = None; + + // unfortunately can't encompass the actual hashmap in a Mutex (Heartbeat isn't Send/Sync), so we'll use a spinlock + static HBS_SPINLOCK: AtomicBool = ATOMIC_BOOL_INIT; + + pub fn lock_and_work(work: F) -> R + where F: FnOnce(&mut Option<*mut HashMap>) -> R { + while HBS_SPINLOCK.compare_and_swap(false, true, Ordering::SeqCst) {} + let result = unsafe { + work(&mut HBS) + }; + HBS_SPINLOCK.store(false, Ordering::SeqCst); + result + } + + /// Callback function used to log the window buffer. + /// When this is called from native C, the heartbeat is safely locked internally and the global lock is held. + /// If calling from this file, you must already hold the global lock! + pub extern fn heartbeat_window_callback(hb: *const HeartbeatContext) { + unsafe { + if let Some(hbs_ptr) = HBS { + for (_, v) in (*hbs_ptr).iter_mut() { + if &v.hb as *const HeartbeatContext == hb { + log_heartbeat_records(v); + } } } }