Skip to content

Commit

Permalink
Remove unsafe, fix start time
Browse files Browse the repository at this point in the history
  • Loading branch information
@Robo210
Robo210 committed Jul 17, 2023
1 parent abdab68 commit 8fc33f4
Show file tree
Hide file tree
Showing 9 changed files with 70 additions and 163 deletions.
1 change: 0 additions & 1 deletion Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ authors = ["Kyle Sabo", "Microsoft"]
crate-type = ["rlib"]

[features]
unsafe = []
global_filter = []
common_schema = []
default = ["common_schema"]
Expand Down
116 changes: 16 additions & 100 deletions src/layer.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use std::marker::PhantomData;
use std::time::SystemTime;
use std::{pin::Pin, sync::Arc};

use tracelogging::Guid;
Expand All @@ -14,42 +15,11 @@ use crate::native::{EventMode, EventWriter};
use crate::values::*;
use crate::{map_level, native};

// This version of the struct is packed such that all the fields are references into a single,
// contiguous allocation. The first byte of the allocation is `layout`.
#[cfg(feature = "unsafe")]
#[repr(C)]
struct EtwLayerData {
fields: &'static [&'static str],
values: &'static mut [ValueTypes],
indexes: &'static mut [u8],
activity_id: [u8; 16], // // if set, byte 0 is 1 and 64-bit span ID in the lower 8 bytes
related_activity_id: [u8; 16], // if set, byte 0 is 1 and 64-bit span ID in the lower 8 bytes
_p: std::ptr::NonNull<std::alloc::Layout>,
}

#[cfg(feature = "unsafe")]
impl Drop for EtwLayerData {
fn drop(&mut self) {
unsafe { std::alloc::dealloc(self._p.as_ptr() as *mut u8, *self._p.as_ptr()) }
}
}

// Everything in the struct is Send + Sync except the ptr::NonNull.
// We never deref the ptr::NonNull except in drop, for which safety comes from safe usage of the struct itself.
// Therefore we can safely tag the whole thing as Send + Sync.
#[cfg(feature = "unsafe")]
unsafe impl Send for EtwLayerData {}
#[cfg(feature = "unsafe")]
unsafe impl Sync for EtwLayerData {}

// This version of the struct uses only safe code, but has 3 separate heap allocations.
#[cfg(not(feature = "unsafe"))]
struct EtwLayerData {
fields: Box<[&'static str]>,
values: Box<[ValueTypes]>,
indexes: Box<[u8]>,
fields: Box<[FieldValueIndex]>,
activity_id: [u8; 16], // // if set, byte 0 is 1 and 64-bit span ID in the lower 8 bytes
related_activity_id: [u8; 16], // if set, byte 0 is 1 and 64-bit span ID in the lower 8 bytes
start_time: SystemTime,
}

#[doc(hidden)]
Expand Down Expand Up @@ -421,68 +391,18 @@ where

let n = metadata.fields().len();

// We want to pack the data that needs to be stored on the span as tightly as possible,
// in order to accommodate as many active spans as possible.
// This is off by default though, since unsafe code is unsafe.
#[cfg(feature = "unsafe")]
let mut data = unsafe {
let layout_layout = std::alloc::Layout::new::<std::alloc::Layout>();
let fields_layout = std::alloc::Layout::array::<&str>(n).unwrap();
let values_layout = std::alloc::Layout::array::<ValueTypes>(n).unwrap();
let indexes_layout = std::alloc::Layout::array::<u8>(n).unwrap();

let (layout, fields_offset) = layout_layout.extend(fields_layout).unwrap();
let (layout, values_offset) = layout.extend(values_layout).unwrap();
let (layout, indexes_offset) = layout.extend(indexes_layout).unwrap();

let block = std::alloc::alloc_zeroed(layout);
if block.is_null() {
panic!();
}

let mut layout_field =
std::ptr::NonNull::new(block as *mut std::alloc::Layout).unwrap();
let fields: &mut [&str] =
std::slice::from_raw_parts_mut(block.add(fields_offset) as *mut &str, n);
let values: &mut [ValueTypes] =
std::slice::from_raw_parts_mut(block.add(values_offset) as *mut ValueTypes, n);
let indexes: &mut [u8] = std::slice::from_raw_parts_mut(block.add(indexes_offset), n);

*layout_field.as_mut() = layout;

let mut i = 0;
for field in metadata.fields().iter() {
fields[i] = field.name();
values[i] = ValueTypes::None;
indexes[i] = i as u8;
i += 1;
}

indexes.sort_by_key(|idx| fields[*idx as usize]);

EtwLayerData {
fields,
values,
indexes,
activity_id: [0; 16],
related_activity_id: [0; 16],
_p: std::ptr::NonNull::new(block as *mut std::alloc::Layout).unwrap(),
let mut data = {
let mut v: Vec<FieldValueIndex> = Vec::with_capacity(n);
v.resize_with(n, Default::default);
for i in 0..n {
v[i].sort_index = i as u8;
}
};

#[cfg(not(feature = "unsafe"))]
let mut data = {
EtwLayerData {
fields: vec![""; n].into_boxed_slice(),
values: vec![ValueTypes::None; n].into_boxed_slice(),
indexes: ([
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21,
22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32,
])[..n]
.to_vec()
.into_boxed_slice(),
fields: v.into_boxed_slice(),
activity_id: [0; 16],
related_activity_id: [0; 16],
start_time: SystemTime::UNIX_EPOCH,
}
};

Expand All @@ -499,9 +419,7 @@ where
};

attrs.values().record(&mut ValueVisitor {
fields: &data.fields,
values: &mut data.values,
indexes: &mut data.indexes,
fields: &mut data.fields,
});

// This will unfortunately box data. It would be ideal if we could avoid this second heap allocation,
Expand Down Expand Up @@ -535,16 +453,17 @@ where
&data.activity_id,
&data.related_activity_id,
&data.fields,
&data.values,
map_level(metadata.level()),
self.default_keyword,
0,
);

data.start_time = timestamp;
}

fn on_exit(&self, id: &span::Id, ctx: tracing_subscriber::layer::Context<'_, S>) {
// A span was exited
let timestamp = std::time::SystemTime::now();
let stop_timestamp = std::time::SystemTime::now();

let span = if let Some(span) = ctx.span(id) {
span
Expand All @@ -564,11 +483,10 @@ where

self.provider.as_ref().span_stop(
&span,
timestamp,
(data.start_time, stop_timestamp),
&data.activity_id,
&data.related_activity_id,
&data.fields,
&data.values,
map_level(metadata.level()),
self.default_keyword,
0,
Expand Down Expand Up @@ -603,9 +521,7 @@ where
};

values.record(&mut ValueVisitor {
fields: &data.fields,
values: &mut data.values,
indexes: &mut data.indexes,
fields: &mut data.fields,
});
}
}
19 changes: 8 additions & 11 deletions src/native/common_schema/etw_cs.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,7 @@ impl crate::native::EventWriter for CommonSchemaProvider {
_timestamp: SystemTime,
_activity_id: &[u8; 16],
_related_activity_id: &[u8; 16],
_fields: &'b [&'static str],
_values: &'b [ValueTypes],
_fields: &'b [crate::values::FieldValueIndex],
_level: u8,
_keyword: u64,
_event_tag: u32,
Expand All @@ -163,11 +162,10 @@ impl crate::native::EventWriter for CommonSchemaProvider {
fn span_stop<'a, 'b, R>(
self: Pin<&Self>,
span: &'b SpanRef<'a, R>,
timestamp: SystemTime,
start_stop_times: (std::time::SystemTime, std::time::SystemTime),
_activity_id: &[u8; 16],
_related_activity_id: &[u8; 16],
fields: &'b [&'static str],
values: &'b [ValueTypes],
fields: &'b [crate::values::FieldValueIndex],
level: u8,
keyword: u64,
event_tag: u32,
Expand Down Expand Up @@ -197,7 +195,7 @@ impl crate::native::EventWriter for CommonSchemaProvider {
eb.add_struct("PartA", 2 /* + exts.len() as u8*/, 0);
{
let time: String =
chrono::DateTime::to_rfc3339(&chrono::DateTime::<chrono::Utc>::from(timestamp));
chrono::DateTime::to_rfc3339(&chrono::DateTime::<chrono::Utc>::from(start_stop_times.1));
eb.add_str8("time", time, OutType::Utf8, 0);

eb.add_struct("ext_dt", 2, 0);
Expand Down Expand Up @@ -238,11 +236,10 @@ impl crate::native::EventWriter for CommonSchemaProvider {

eb.add_str8("name", span_name, OutType::Utf8, 0);

// TODO
eb.add_str8(
"startTime",
&chrono::DateTime::to_rfc3339(&chrono::DateTime::<chrono::Utc>::from(
timestamp,
start_stop_times.0,
)),
OutType::Utf8,
0,
Expand All @@ -255,14 +252,14 @@ impl crate::native::EventWriter for CommonSchemaProvider {
{
let mut pfv = CommonSchemaPartCBuilder { eb: eb.deref_mut() };

for (f, v) in fields.iter().zip(values.iter()) {
for f in fields {
<CommonSchemaPartCBuilder<'_> as AddFieldAndValue<
CommonSchemaPartCBuilder<'_>,
>>::add_field_value(
&mut pfv,
&FieldAndValue {
field_name: f,
value: v,
field_name: f.field,
value: &f.value,
},
);
}
Expand Down
19 changes: 8 additions & 11 deletions src/native/common_schema/user_events_cs.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,7 @@ impl crate::native::EventWriter for CommonSchemaProvider {
_timestamp: SystemTime,
_activity_id: &[u8; 16],
_related_activity_id: &[u8; 16],
_fields: &'b [&'static str],
_values: &'b [ValueTypes],
_fields: &'b [crate::values::FieldValueIndex],
_level: u8,
_keyword: u64,
_event_tag: u32,
Expand All @@ -179,11 +178,10 @@ impl crate::native::EventWriter for CommonSchemaProvider {
fn span_stop<'a, 'b, R>(
self: Pin<&Self>,
span: &'b SpanRef<'a, R>,
timestamp: SystemTime,
start_stop_times: (std::time::SystemTime, std::time::SystemTime),
_activity_id: &[u8; 16],
_related_activity_id: &[u8; 16],
fields: &'b [&'static str],
values: &'b [ValueTypes],
fields: &'b [crate::values::FieldValueIndex],
level: u8,
keyword: u64,
event_tag: u32,
Expand Down Expand Up @@ -219,7 +217,7 @@ impl crate::native::EventWriter for CommonSchemaProvider {
eb.add_struct("PartA", 2 /* + exts.len() as u8*/, 0);
{
let time: String =
chrono::DateTime::to_rfc3339(&chrono::DateTime::<chrono::Utc>::from(timestamp));
chrono::DateTime::to_rfc3339(&chrono::DateTime::<chrono::Utc>::from(start_stop_times.1));
eb.add_str("time", time, FieldFormat::Default, 0);

eb.add_struct("ext_dt", 2, 0);
Expand Down Expand Up @@ -260,11 +258,10 @@ impl crate::native::EventWriter for CommonSchemaProvider {

eb.add_str("name", span_name, FieldFormat::Default, 0);

// TODO
eb.add_str(
"startTime",
&chrono::DateTime::to_rfc3339(&chrono::DateTime::<chrono::Utc>::from(
timestamp,
start_stop_times.0,
)),
FieldFormat::Default,
0,
Expand All @@ -277,14 +274,14 @@ impl crate::native::EventWriter for CommonSchemaProvider {
{
let mut pfv = CommonSchemaPartCBuilder { eb: eb.deref_mut() };

for (f, v) in fields.iter().zip(values.iter()) {
for f in fields {
<CommonSchemaPartCBuilder<'_> as AddFieldAndValue<
CommonSchemaPartCBuilder<'_>,
>>::add_field_value(
&mut pfv,
&FieldAndValue {
field_name: f,
value: v,
field_name: f.field,
value: &f.value,
},
);
}
Expand Down
22 changes: 10 additions & 12 deletions src/native/etw.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,7 @@ impl super::EventWriter for Provider {
timestamp: SystemTime,
activity_id: &[u8; 16],
related_activity_id: &[u8; 16],
fields: &'b [&'static str],
values: &'b [ValueTypes],
fields: &'b [crate::values::FieldValueIndex],
level: u8,
keyword: u64,
event_tag: u32,
Expand All @@ -183,10 +182,10 @@ impl super::EventWriter for Provider {

let mut pfv = PayloadFieldVisitor { eb: eb.deref_mut() };

for (f, v) in fields.iter().zip(values.iter()) {
for f in fields {
<PayloadFieldVisitor<'_> as AddFieldAndValue<PayloadFieldVisitor<'_>>>::add_field_value(&mut pfv, &FieldAndValue {
field_name: f,
value: v,
field_name: f.field,
value: &f.value,
});
}

Expand All @@ -211,11 +210,10 @@ impl super::EventWriter for Provider {
fn span_stop<'a, 'b, R>(
self: Pin<&Self>,
span: &'b SpanRef<'a, R>,
timestamp: SystemTime,
start_stop_times: (std::time::SystemTime, std::time::SystemTime),
activity_id: &[u8; 16],
related_activity_id: &[u8; 16],
fields: &'b [&'static str],
values: &'b [ValueTypes],
fields: &'b [crate::values::FieldValueIndex],
level: u8,
keyword: u64,
event_tag: u32,
Expand All @@ -232,17 +230,17 @@ impl super::EventWriter for Provider {

eb.add_systemtime(
"stop time",
&Into::<Win32SystemTime>::into(timestamp).st,
&Into::<Win32SystemTime>::into(start_stop_times.1).st,
OutType::DateTimeUtc,
0,
);

let mut pfv = PayloadFieldVisitor { eb: eb.deref_mut() };

for (f, v) in fields.iter().zip(values.iter()) {
for f in fields {
<PayloadFieldVisitor<'_> as AddFieldAndValue<PayloadFieldVisitor<'_>>>::add_field_value(&mut pfv, &FieldAndValue {
field_name: f,
value: v,
field_name: f.field,
value: &f.value,
});
}

Expand Down
Loading

0 comments on commit 8fc33f4

Please sign in to comment.