Magic-Mirror/gecko-dev
1
0
Fork 0
mirror of https://github.com/mozilla/gecko-dev.git synced 2024-11-23 12:51:06 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Bug 1920704 - Let the C++ side fully handle the memory management of MarkerSchema objects in the Rust API r=mstange,profiler-reviewers

Browse Source 
Previously we were using a `Pin` to create a memory location for the C++
MarkerSchema object in Rust, then we were creating this object inside that
location. To make sure that this object doesn't move around, we were using
`Pin`. But `Pin` only guarantees when it's managing a Rust object and doesn't
really guarantee when managing a foreign object like MarkerSchema from C++. So
this was creating an undefined behavior where it was surfacing in one of our
tests.

This patch changing this behavior, so now C++ side is fully responsible of
constructing the object in the heap and destructing when we are done using it.

I'm not so sure why this gets fixed when we pause the profiler before we
capture the profile, but this seems to fix the issue.

Differential Revision: https://phabricator.services.mozilla.com/D223766
This commit is contained in:
Nazım Can Altınova 2024-09-27 13:01:18 +00:00
parent 69a760a670
commit 028d69f48f
4 changed files with 38 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
tools/profiler/core/ProfilerBindings.cpp
View File

@ -131,26 +131,29 @@ void gecko_profiler_destruct_marker_timing(
#endif
}
void gecko_profiler_construct_marker_schema(
mozilla::MarkerSchema* aMarkerSchema,
mozilla::MarkerSchema* gecko_profiler_construct_marker_schema(
const mozilla::MarkerSchema::Location* aLocations, size_t aLength) {
#ifdef MOZ_GECKO_PROFILER
new (aMarkerSchema) mozilla::MarkerSchema(aLocations, aLength);
return new mozilla::MarkerSchema(aLocations, aLength);
#else
return nullptr;
#endif
}
void gecko_profiler_construct_marker_schema_with_special_front_end_location(
mozilla::MarkerSchema* aMarkerSchema) {
mozilla::MarkerSchema*
gecko_profiler_construct_marker_schema_with_special_front_end_location() {
#ifdef MOZ_GECKO_PROFILER
new (aMarkerSchema)
mozilla::MarkerSchema(mozilla::MarkerSchema::SpecialFrontendLocation{});
return new mozilla::MarkerSchema(
mozilla::MarkerSchema::SpecialFrontendLocation{});
#else
return nullptr;
#endif
}
void gecko_profiler_destruct_marker_schema(
mozilla::MarkerSchema* aMarkerSchema) {
#ifdef MOZ_GECKO_PROFILER
aMarkerSchema->~MarkerSchema();
delete aMarkerSchema;
#endif
}

7
tools/profiler/public/ProfilerBindings.h
View File

@ -75,11 +75,10 @@ void gecko_profiler_destruct_marker_timing(
mozilla::MarkerTiming* aMarkerTiming);
// MarkerSchema constructors and destructor.
void gecko_profiler_construct_marker_schema(
mozilla::MarkerSchema* aMarkerSchema,
mozilla::MarkerSchema* gecko_profiler_construct_marker_schema(
const mozilla::MarkerSchema::Location* aLocations, size_t aLength);
void gecko_profiler_construct_marker_schema_with_special_front_end_location(
mozilla::MarkerSchema* aMarkerSchema);
mozilla::MarkerSchema*
gecko_profiler_construct_marker_schema_with_special_front_end_location();
void gecko_profiler_destruct_marker_schema(
mozilla::MarkerSchema* aMarkerSchema);

5
tools/profiler/rust-api/src/gecko_bindings/glue.rs
View File

@ -7,7 +7,6 @@ use crate::json_writer::JSONWriter;
use crate::marker::deserializer_tags_state::{
get_marker_type_functions_read_guard, MarkerTypeFunctions,
};
use std::ops::DerefMut;
use std::os::raw::{c_char, c_void};
#[no_mangle]
@ -40,13 +39,13 @@ pub unsafe extern "C" fn gecko_profiler_stream_marker_schemas(
for funcs in marker_type_functions.iter() {
let marker_name = (funcs.marker_type_name_fn)();
let mut marker_schema = (funcs.marker_type_display_fn)();
let marker_schema = (funcs.marker_type_display_fn)();
bindings::gecko_profiler_marker_schema_stream(
json_writer,
marker_name.as_ptr() as *const c_char,
marker_name.len(),
marker_schema.pin.deref_mut().as_mut_ptr(),
marker_schema.ptr,
streamed_names_set,
)
}

51
tools/profiler/rust-api/src/marker/schema.rs
View File

@ -5,10 +5,7 @@
//! [`MarkerSchema`] and other enums that will be used by `MarkerSchema`.
use crate::gecko_bindings::{bindings, structs::mozilla};
use std::mem::MaybeUninit;
use std::ops::DerefMut;
use std::os::raw::c_char;
use std::pin::Pin;
/// Marker locations to be displayed in the profiler front-end.
pub type Location = mozilla::MarkerSchema_Location;
@ -27,34 +24,30 @@ pub type Searchable = mozilla::MarkerSchema_Searchable;
/// It's a RAII object that constructs and destroys a C++ MarkerSchema object
/// pointed to a specified reference.
pub struct MarkerSchema {
pub(crate) pin: Pin<Box<MaybeUninit<mozilla::MarkerSchema>>>,
pub(crate) ptr: *mut mozilla::MarkerSchema,
}
impl MarkerSchema {
// Initialize a marker schema with the given `Location`s.
pub fn new(locations: &[Location]) -> Self {
let mut marker_schema = Box::pin(std::mem::MaybeUninit::<mozilla::MarkerSchema>::uninit());
unsafe {
bindings::gecko_profiler_construct_marker_schema(
marker_schema.deref_mut().as_mut_ptr(),
locations.as_ptr(),
locations.len(),
);
MarkerSchema {
ptr: unsafe {
bindings::gecko_profiler_construct_marker_schema(
locations.as_ptr(),
locations.len(),
)
},
}
MarkerSchema { pin: marker_schema }
}
/// Marker schema for types that have special frontend handling.
/// Nothing else should be set in this case.
pub fn new_with_special_frontend_location() -> Self {
let mut marker_schema = Box::pin(std::mem::MaybeUninit::<mozilla::MarkerSchema>::uninit());
unsafe {
bindings::gecko_profiler_construct_marker_schema_with_special_front_end_location(
marker_schema.deref_mut().as_mut_ptr(),
);
MarkerSchema {
ptr: unsafe {
bindings::gecko_profiler_construct_marker_schema_with_special_front_end_location()
},
}
MarkerSchema { pin: marker_schema }
}
/// Optional label in the marker chart.
@ -64,7 +57,7 @@ impl MarkerSchema {
pub fn set_chart_label(&mut self, label: &str) -> &mut Self {
unsafe {
bindings::gecko_profiler_marker_schema_set_chart_label(
self.pin.deref_mut().as_mut_ptr(),
self.ptr,
label.as_ptr() as *const c_char,
label.len(),
);
@ -79,7 +72,7 @@ impl MarkerSchema {
pub fn set_tooltip_label(&mut self, label: &str) -> &mut Self {
unsafe {
bindings::gecko_profiler_marker_schema_set_tooltip_label(
self.pin.deref_mut().as_mut_ptr(),
self.ptr,
label.as_ptr() as *const c_char,
label.len(),
);
@ -94,7 +87,7 @@ impl MarkerSchema {
pub fn set_table_label(&mut self, label: &str) -> &mut Self {
unsafe {
bindings::gecko_profiler_marker_schema_set_table_label(
self.pin.deref_mut().as_mut_ptr(),
self.ptr,
label.as_ptr() as *const c_char,
label.len(),
);
@ -109,7 +102,7 @@ impl MarkerSchema {
pub fn set_all_labels(&mut self, label: &str) -> &mut Self {
unsafe {
bindings::gecko_profiler_marker_schema_set_all_labels(
self.pin.deref_mut().as_mut_ptr(),
self.ptr,
label.as_ptr() as *const c_char,
label.len(),
);
@ -132,7 +125,7 @@ impl MarkerSchema {
pub fn add_key_format(&mut self, key: &str, format: Format) -> &mut Self {
unsafe {
bindings::gecko_profiler_marker_schema_add_key_format(
self.pin.deref_mut().as_mut_ptr(),
self.ptr,
key.as_ptr() as *const c_char,
key.len(),
format,
@ -148,7 +141,7 @@ impl MarkerSchema {
pub fn add_key_label_format(&mut self, key: &str, label: &str, format: Format) -> &mut Self {
unsafe {
bindings::gecko_profiler_marker_schema_add_key_label_format(
self.pin.deref_mut().as_mut_ptr(),
self.ptr,
key.as_ptr() as *const c_char,
key.len(),
label.as_ptr() as *const c_char,
@ -170,7 +163,7 @@ impl MarkerSchema {
) -> &mut Self {
unsafe {
bindings::gecko_profiler_marker_schema_add_key_format_searchable(
self.pin.deref_mut().as_mut_ptr(),
self.ptr,
key.as_ptr() as *const c_char,
key.len(),
format,
@ -195,7 +188,7 @@ impl MarkerSchema {
) -> &mut Self {
unsafe {
bindings::gecko_profiler_marker_schema_add_key_label_format_searchable(
self.pin.deref_mut().as_mut_ptr(),
self.ptr,
key.as_ptr() as *const c_char,
key.len(),
label.as_ptr() as *const c_char,
@ -213,7 +206,7 @@ impl MarkerSchema {
pub fn add_static_label_value(&mut self, label: &str, value: &str) -> &mut Self {
unsafe {
bindings::gecko_profiler_marker_schema_add_static_label_value(
self.pin.deref_mut().as_mut_ptr(),
self.ptr,
label.as_ptr() as *const c_char,
label.len(),
value.as_ptr() as *const c_char,
@ -227,7 +220,7 @@ impl MarkerSchema {
impl Drop for MarkerSchema {
fn drop(&mut self) {
unsafe {
bindings::gecko_profiler_destruct_marker_schema(self.pin.deref_mut().as_mut_ptr());
bindings::gecko_profiler_destruct_marker_schema(self.ptr);
}
}
}