Skip to content

Commit 7ae15a5

Browse files
morrisonlevimorrisonlevi
authored
Profiling: fix SystemSettings initialization (#3579)
* fix(profiling): SystemSettings initialization * test(profiling): allocation_sampling_distance regression
1 parent 48c4254 commit 7ae15a5

8 files changed

Lines changed: 169 additions & 78 deletions

File tree

‎profiling/src/allocation/allocation_ge84.rs‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -146,15 +146,9 @@ lazy_static! {
146146
}
147147

148148
pub fn first_rinit_should_disable_due_to_jit() -> bool {
149-
if *JIT_ENABLED
149+
*JIT_ENABLED
150150
&& zend::PHP_VERSION_ID >= 80400
151151
&& (80400..80406).contains(&crate::RUNTIME_PHP_VERSION_ID.load(Relaxed))
152-
{
153-
error!("Memory allocation profiling will be disabled as long as JIT is active. To enable allocation profiling disable JIT or upgrade PHP to at least version 8.4.7. See https://github.com/DataDog/dd-trace-php/pull/3199");
154-
true
155-
} else {
156-
false
157-
}
158152
}
159153

160154
/// This initializes the thread locale variable `ZEND_MM_STATE` with respect to the currently

‎profiling/src/allocation/allocation_le83.rs‎

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ use crate::{RefCellExt, PROFILER_NAME, REQUEST_LOCALS};
77
use core::{cell::Cell, ptr};
88
use lazy_static::lazy_static;
99
use libc::{c_char, c_int, c_void, size_t};
10-
use log::{debug, error, trace, warn};
10+
use log::{debug, trace, warn};
1111
use std::sync::atomic::Ordering::Relaxed;
1212

1313
#[cfg(feature = "debug_stats")]
@@ -132,19 +132,9 @@ pub fn alloc_prof_ginit() {
132132
}
133133

134134
pub fn first_rinit_should_disable_due_to_jit() -> bool {
135-
if NEEDS_RUN_TIME_CHECK_FOR_ENABLED_JIT
135+
NEEDS_RUN_TIME_CHECK_FOR_ENABLED_JIT
136136
&& alloc_prof_needs_disabled_for_jit(crate::RUNTIME_PHP_VERSION_ID.load(Relaxed))
137137
&& *JIT_ENABLED
138-
{
139-
if zend::PHP_VERSION_ID >= 80400 {
140-
error!("Memory allocation profiling will be disabled as long as JIT is active. To enable allocation profiling disable JIT or upgrade PHP to at least version 8.4.7. See https://github.com/DataDog/dd-trace-php/pull/3199");
141-
} else {
142-
error!("Memory allocation profiling will be disabled as long as JIT is active. To enable allocation profiling disable JIT or upgrade PHP to at least version 8.1.21 or 8.2.8. See https://github.com/DataDog/dd-trace-php/pull/2088");
143-
}
144-
true
145-
} else {
146-
false
147-
}
148138
}
149139

150140
pub fn alloc_prof_rinit() {

‎profiling/src/allocation/mod.rs‎

Lines changed: 34 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -11,24 +11,28 @@ use crate::bindings::{self as zend};
1111
use crate::profiling::Profiler;
1212
use crate::{RefCellExt, REQUEST_LOCALS};
1313
use libc::size_t;
14-
use log::{debug, error, trace};
14+
use log::{debug, trace};
1515
use rand_distr::{Distribution, Poisson};
1616
use std::ffi::c_void;
17+
use std::num::{NonZero, NonZeroU32, NonZeroU64};
1718
use std::sync::atomic::{AtomicU64, Ordering};
1819

20+
use crate::config::SystemSettings;
1921
#[cfg(not(php_zts))]
2022
use rand::rngs::StdRng;
2123
#[cfg(php_zts)]
2224
use rand::rngs::ThreadRng;
2325
#[cfg(not(php_zts))]
2426
use rand::SeedableRng;
2527

26-
/// Default sampling interval in bytes (4MB)
27-
pub const DEFAULT_ALLOCATION_SAMPLING_INTERVAL: u64 = 1024 * 4096;
28+
/// Default sampling interval in bytes (4 MiB).
29+
// SAFETY: value is > 0.
30+
pub const DEFAULT_ALLOCATION_SAMPLING_INTERVAL: NonZeroU32 =
31+
unsafe { NonZero::new_unchecked(1024 * 4096) };
2832

29-
/// Sampling distance feed into poison sampling algo
33+
/// Sampling distance feed into poison sampling algo. This must be > 0.
3034
pub static ALLOCATION_PROFILING_INTERVAL: AtomicU64 =
31-
AtomicU64::new(DEFAULT_ALLOCATION_SAMPLING_INTERVAL);
35+
AtomicU64::new(DEFAULT_ALLOCATION_SAMPLING_INTERVAL.get() as u64);
3236

3337
/// This will store the count of allocations (including reallocations) during
3438
/// a profiling period. This will overflow when doing more than u64::MAX
@@ -53,10 +57,9 @@ pub struct AllocationProfilingStats {
5357
}
5458

5559
impl AllocationProfilingStats {
56-
fn new() -> AllocationProfilingStats {
57-
// Safety: this will only error if lambda <= 0
58-
let poisson =
59-
Poisson::new(ALLOCATION_PROFILING_INTERVAL.load(Ordering::Relaxed) as f64).unwrap();
60+
fn new(sampling_distance: NonZeroU64) -> AllocationProfilingStats {
61+
// SAFETY: this will only error if lambda <= 0, and it's NonZeroU64.
62+
let poisson = unsafe { Poisson::new(sampling_distance.get() as f64).unwrap_unchecked() };
6063
let mut stats = AllocationProfilingStats {
6164
next_sample: 0,
6265
poisson,
@@ -105,30 +108,35 @@ pub fn alloc_prof_startup() {
105108
allocation_le83::alloc_prof_startup();
106109
}
107110

108-
pub fn alloc_prof_first_rinit() {
109-
let (allocation_enabled, sampling_distance) = REQUEST_LOCALS
110-
.try_with_borrow(|locals| {
111-
let settings = locals.system_settings();
112-
(settings.profiling_allocation_enabled, settings.profiling_allocation_sampling_distance)
113-
})
114-
.unwrap_or_else(|err| {
115-
error!("Allocation profiling first rinit failed because it failed to borrow the request locals. Please report this to Datadog: {err}");
116-
(false, DEFAULT_ALLOCATION_SAMPLING_INTERVAL as u32)
117-
});
111+
pub fn first_rinit(settings: &SystemSettings) {
112+
if !settings.profiling_allocation_enabled {
113+
return;
114+
}
118115

119-
if !allocation_enabled {
116+
let sampling_distance = settings.profiling_allocation_sampling_distance;
117+
ALLOCATION_PROFILING_INTERVAL.store(sampling_distance.get() as u64, Ordering::Relaxed);
118+
119+
trace!("Memory allocation profiling initialized with a sampling distance of {sampling_distance} bytes.");
120+
}
121+
122+
/// # Safety
123+
///
124+
/// Must be called exactly once per extension minit.
125+
pub unsafe fn minit(settings: &SystemSettings) {
126+
if !settings.profiling_allocation_enabled {
120127
return;
121128
}
122129

123-
ALLOCATION_PROFILING_INTERVAL.store(sampling_distance as u64, Ordering::Relaxed);
130+
let sampling_distance = settings.profiling_allocation_sampling_distance;
131+
ALLOCATION_PROFILING_INTERVAL.store(sampling_distance.get() as u64, Ordering::Relaxed);
132+
133+
// SAFETY: called in minit.
134+
unsafe { profiling_stats::minit(sampling_distance.into()) };
124135

125-
trace!(
126-
"Memory allocation profiling initialized with a sampling distance of {} bytes.",
127-
ALLOCATION_PROFILING_INTERVAL.load(Ordering::Relaxed)
128-
);
136+
trace!("Memory allocation profiling initialized with a sampling distance of {sampling_distance} bytes.");
129137
}
130138

131-
pub fn alloc_prof_rinit() {
139+
pub fn rinit() {
132140
let allocation_enabled = REQUEST_LOCALS
133141
.try_with_borrow(|locals| locals.system_settings().profiling_allocation_enabled)
134142
.unwrap_or_else(|err| {

‎profiling/src/allocation/profiling_stats.rs‎

Lines changed: 32 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,17 +3,18 @@
33
//! performance sensitive. It is encapsulated so that some unsafe techniques
44
//! can be used but expose a relatively safe API.
55
6-
use super::AllocationProfilingStats;
6+
use super::{AllocationProfilingStats, ALLOCATION_PROFILING_INTERVAL};
77
use libc::size_t;
88
use std::mem::MaybeUninit;
99

10-
#[cfg(php_zts)]
11-
use std::cell::UnsafeCell;
12-
1310
#[cfg(php_zend_mm_set_custom_handlers_ex)]
1411
use super::allocation_ge84;
1512
#[cfg(not(php_zend_mm_set_custom_handlers_ex))]
1613
use super::allocation_le83;
14+
#[cfg(php_zts)]
15+
use std::cell::UnsafeCell;
16+
use std::num::NonZeroU64;
17+
use std::sync::atomic::Ordering;
1718

1819
#[cfg(php_zts)]
1920
thread_local! {
@@ -107,12 +108,15 @@ pub fn allocation_profiling_stats_should_collect(len: size_t) -> bool {
107108
/// Must be called once per PHP thread ginit.
108109
pub unsafe fn ginit() {
109110
// SAFETY:
110-
// 1. During ginit, there will not be any other borrows.
111-
// 2. This closure will not make new borrows.
111+
// 1. During ginit, there will not be any other borrows to stats.
112+
// 2. This closure will not make new borrows to stats.
112113
// 3. This is not during the thread-local destructor.
113114
unsafe {
114115
allocation_profiling_stats_mut(|uninit| {
115-
uninit.write(AllocationProfilingStats::new());
116+
let interval = ALLOCATION_PROFILING_INTERVAL.load(Ordering::Relaxed);
117+
// SAFETY: ALLOCATION_PROFILING_INTERVAL must always be > 0.
118+
let nonzero = NonZeroU64::new_unchecked(interval);
119+
uninit.write(AllocationProfilingStats::new(nonzero));
116120
})
117121
};
118122

@@ -122,6 +126,27 @@ pub unsafe fn ginit() {
122126
allocation_ge84::alloc_prof_ginit();
123127
}
124128

129+
/// Initializes the allocation profiler's globals with the provided sampling
130+
/// distance.
131+
///
132+
/// # Safety
133+
///
134+
/// Must be called once per PHP thread minit, unless the allocation profiling
135+
/// is disabled, in which case it can be skipped.
136+
pub unsafe fn minit(sampling_distance: NonZeroU64) {
137+
// SAFETY:
138+
// 1. During minit, there will not be any other borrows.
139+
// 2. This closure will not make new borrows.
140+
// 3. This is not during the thread-local destructor.
141+
unsafe {
142+
allocation_profiling_stats_mut(|uninit| {
143+
// SAFETY: previously initialized in ginit, we're just
144+
// re-initializing it because we now have config
145+
*uninit.assume_init_mut() = AllocationProfilingStats::new(sampling_distance);
146+
})
147+
};
148+
}
149+
125150
/// Shuts down the allocation profiler's globals.
126151
///
127152
/// # Safety

‎profiling/src/config.rs‎

Lines changed: 48 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
use crate::allocation;
21
use crate::bindings::zai_config_type::*;
32
use crate::bindings::{
43
datadog_php_profiling_copy_string_view_into_zval, ddog_php_prof_get_memoized_config,
@@ -7,16 +6,18 @@ use crate::bindings::{
76
StringError, ZaiStr, IS_FALSE, IS_LONG, IS_TRUE, ZAI_CONFIG_NAME_BUFSIZ, ZEND_INI_DISPLAY_ORIG,
87
};
98
use crate::zend::zai_str_from_zstr;
9+
use crate::{allocation, bindings, zend};
1010
use core::fmt::{Display, Formatter};
1111
use core::mem::transmute;
1212
use core::ptr;
1313
use core::str::FromStr;
1414
use libc::{c_char, c_int};
1515
use libdd_common::tag::{parse_tags, Tag};
1616
pub use libdd_profiling::exporter::Uri;
17-
use log::{debug, warn, LevelFilter};
17+
use log::{debug, error, warn, LevelFilter};
1818
use std::borrow::Cow;
1919
use std::ffi::CString;
20+
use std::num::NonZeroU32;
2021
use std::path::{Path, PathBuf};
2122

2223
#[derive(Copy, Clone, Debug, Default)]
@@ -43,7 +44,7 @@ pub struct SystemSettings {
4344
pub profiling_endpoint_collection_enabled: bool,
4445
pub profiling_experimental_cpu_time_enabled: bool,
4546
pub profiling_allocation_enabled: bool,
46-
pub profiling_allocation_sampling_distance: u32,
47+
pub profiling_allocation_sampling_distance: NonZeroU32,
4748
pub profiling_timeline_enabled: bool,
4849
pub profiling_exception_enabled: bool,
4950
pub profiling_exception_message_enabled: bool,
@@ -67,7 +68,7 @@ impl SystemSettings {
6768
profiling_endpoint_collection_enabled: false,
6869
profiling_experimental_cpu_time_enabled: false,
6970
profiling_allocation_enabled: false,
70-
profiling_allocation_sampling_distance: u32::MAX,
71+
profiling_allocation_sampling_distance: NonZeroU32::MAX,
7172
profiling_timeline_enabled: false,
7273
profiling_exception_enabled: false,
7374
profiling_exception_message_enabled: false,
@@ -82,8 +83,7 @@ impl SystemSettings {
8283

8384
/// # Safety
8485
/// This function must only be called after ZAI config has been
85-
/// initialized in first rinit, and before config is uninitialized in
86-
/// shutdown.
86+
/// initialized, and before config is uninitialized in shutdown.
8787
unsafe fn new() -> SystemSettings {
8888
// Select agent URI/UDS.
8989
let agent_host = agent_host();
@@ -130,22 +130,19 @@ impl SystemSettings {
130130
unsafe fn on_first_request() {
131131
let mut system_settings = SystemSettings::new();
132132

133-
// Initialize logging before allocation's rinit, as it logs.
134-
cfg_if::cfg_if! {
135-
if #[cfg(debug_assertions)] {
136-
log::set_max_level(system_settings.profiling_log_level);
137-
} else {
138-
crate::logging::log_init(system_settings.profiling_log_level);
139-
}
140-
}
141-
142133
// Work around version-specific issues.
143134
#[cfg(not(php_zend_mm_set_custom_handlers_ex))]
144135
if allocation::allocation_le83::first_rinit_should_disable_due_to_jit() {
136+
if zend::PHP_VERSION_ID >= 80400 {
137+
error!("Memory allocation profiling will be disabled as long as JIT is active. To enable allocation profiling disable JIT or upgrade PHP to at least version 8.4.7. See https://github.com/DataDog/dd-trace-php/pull/3199");
138+
} else {
139+
error!("Memory allocation profiling will be disabled as long as JIT is active. To enable allocation profiling disable JIT or upgrade PHP to at least version 8.1.21 or 8.2.8. See https://github.com/DataDog/dd-trace-php/pull/2088");
140+
}
145141
system_settings.profiling_allocation_enabled = false;
146142
}
147143
#[cfg(php_zend_mm_set_custom_handlers_ex)]
148144
if allocation::allocation_ge84::first_rinit_should_disable_due_to_jit() {
145+
error!("Memory allocation profiling will be disabled as long as JIT is active. To enable allocation profiling disable JIT or upgrade PHP to at least version 8.4.7. See https://github.com/DataDog/dd-trace-php/pull/3199");
149146
system_settings.profiling_allocation_enabled = false;
150147
}
151148

@@ -475,7 +472,8 @@ static DEFAULT_SYSTEM_SETTINGS: SystemSettings = SystemSettings {
475472
profiling_endpoint_collection_enabled: true,
476473
profiling_experimental_cpu_time_enabled: true,
477474
profiling_allocation_enabled: true,
478-
profiling_allocation_sampling_distance: 1024 * 4096,
475+
// SAFETY: value is > 0.
476+
profiling_allocation_sampling_distance: unsafe { NonZeroU32::new_unchecked(1024 * 4096) },
479477
profiling_timeline_enabled: true,
480478
profiling_exception_enabled: true,
481479
profiling_exception_message_enabled: false,
@@ -542,11 +540,16 @@ unsafe fn profiling_allocation_enabled() -> bool {
542540
/// # Safety
543541
/// This function must only be called after config has been initialized in
544542
/// rinit, and before it is uninitialized in mshutdown.
545-
unsafe fn profiling_allocation_sampling_distance() -> u32 {
546-
get_system_uint32(
543+
unsafe fn profiling_allocation_sampling_distance() -> NonZeroU32 {
544+
let int = get_system_uint32(
547545
ProfilingAllocationSamplingDistance,
548-
DEFAULT_SYSTEM_SETTINGS.profiling_allocation_sampling_distance,
549-
)
546+
DEFAULT_SYSTEM_SETTINGS
547+
.profiling_allocation_sampling_distance
548+
.get(),
549+
);
550+
// SAFETY: ProfilingAllocationSamplingDistance uses parser that ensures a
551+
// non-zero value.
552+
unsafe { NonZeroU32::new_unchecked(int) }
550553
}
551554

552555
/// # Safety
@@ -1233,8 +1236,31 @@ pub(crate) fn minit(module_number: libc::c_int) {
12331236
);
12341237
assert!(tmp); // It's literally return true in the source.
12351238

1236-
let system_settings = &mut *ptr::addr_of_mut!(SYSTEM_SETTINGS);
1237-
*system_settings = SystemSettings::initial();
1239+
// We set this so that we can access config for system INI settings during
1240+
// minit, for example for allocation_sampling_distance.
1241+
let in_request = false;
1242+
bindings::zai_config_first_time_rinit(in_request);
1243+
1244+
// SAFETY: just initialized zai config.
1245+
let mut system_settings = SystemSettings::new();
1246+
1247+
// Initialize logging before allocation's rinit, as it logs.
1248+
cfg_if::cfg_if! {
1249+
if #[cfg(debug_assertions)] {
1250+
log::set_max_level(system_settings.profiling_log_level);
1251+
} else {
1252+
crate::logging::log_init(system_settings.profiling_log_level);
1253+
}
1254+
}
1255+
1256+
SystemSettings::log_state(
1257+
(*ptr::addr_of!(SYSTEM_SETTINGS)).state,
1258+
system_settings.state,
1259+
"the module was initialized",
1260+
);
1261+
ptr::addr_of_mut!(SYSTEM_SETTINGS).swap(&mut system_settings);
1262+
1263+
allocation::minit(&*ptr::addr_of!(SYSTEM_SETTINGS))
12381264
}
12391265
}
12401266

‎profiling/src/lib.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -692,7 +692,7 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult {
692692
#[cfg(all(feature = "io_profiling", target_os = "linux"))]
693693
io::io_prof_first_rinit();
694694

695-
allocation::alloc_prof_first_rinit();
695+
allocation::first_rinit(system_settings);
696696
});
697697

698698
Profiler::init(system_settings);
@@ -742,7 +742,7 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult {
742742
TAGS.set(Arc::default());
743743
}
744744

745-
allocation::alloc_prof_rinit();
745+
allocation::rinit();
746746

747747
// SAFETY: called after config is initialized.
748748
unsafe { timeline::timeline_rinit() };

0 commit comments

Comments
 (0)