refactor(profiling): rework internal types #214

morrisonlevi · 2023-08-12T21:06:44Z

What does this PR do?

This continues pulling out things from profiling/src/profiling/mod.rs and puts them into profiling/src/profile/internal/*. Additionally, it introduces traits for a few concepts that were already there, just unnamed (and not tidy):

Id, e.g. FunctionId, MappingId, LocationId, StackTraceId, etc.
Item, e.g. Function, Mapping, Location, StackTrace, etc.
PprofItem, e.g. Function, Mapping, Location, etc but not StackTrace.

The DedupExt trait was simplified, which is partly possible because I understand Rust better now, but also because of the new traits. It enforces the max-size and will panic if the containers get too big. Previously, only mappings checked for being full, which is a bit ironic because it's the least likely in practice to overflow.

Hopefully this explains why it's such a large diff: all these things are connected, although I probably snuck in a change or two that didn't strictly have to be in there (it's hard when it's this big to notice).

Motivation

The goals are to increase cleanliness and separation between the different representations of profiles. We now have api::*, internal::*, and pprof::*, which are fairly contained and separated. There should probably be more cleaning up of the internal::* parts, but this PR is big enough already.

Additional Notes

I would start the review by looking at profiling/src/profile/internal/mod.rs, and reading the 3 trait definitions first.

How to test the change?

Since this targets the internal representation, existing tests ought to be good enough.

This continues pulling out things from src/profiling/mod.rs and puts them into `internal::*`. Additionally, it introduces traits for a few concepts that were already there, just unnamed (and not tidy): - Id, e.g. FunctionId, MappingId, LocationId, StackTraceId, etc. - Item, e.g. Function, Mapping, Location, StackTrace, etc. - PprofItem, e.g. Function, Mapping, Location, etc but not StackTrace. The DedupExt trait was simplified, which is partly possible because I understand Rust better now, but also because of the new traits. It enforces the max-size and will panic if the containers get too big. Previously, only mappings checked for being full, which is a bit ironic because it's the least likely in practice to overflow. Hopefully this explains why it's such a large diff: all these things are connected, although I probably snuck in a change or two that didn't strictly have to be in there (it's hard when it's this big to notice).

sanchda · 2023-08-13T01:00:57Z

profiling/src/profile/internal/mapping.rs

+    /// Address at which the binary (or DLL) is loaded into memory.
+    pub memory_start: u64,
+    /// The limit of the address range occupied by this mapping.
+    pub memory_limit: u64,


As it stands now, we're stuck with this; but if we change from a [start, end) formulation to a [start, start + size) formulation, we could realistically squeeze the size parameter into a u32. These mappings are actually ELF sections (otherwise including both start + file offset is kinda weird), and they probably need to be executable unless we're providing location information for non-code resources (why lol), soooo I personally think u32 would be reasonable there.

Obviously this would be inappropriate for this PR, just making a comment.

What about needing to be executable makes it reasonable to expect it to fit in a u32?

profiling/src/profile/internal/mapping.rs

sanchda · 2023-08-13T01:03:08Z

profiling/src/profile/internal/mapping.rs

+    /// The limit of the address range occupied by this mapping.
+    pub memory_limit: u64,
+    /// Offset in the binary that corresponds to the first mapped address.
+    pub file_offset: u64,


Realistically, I think this can be bound to a u32. This combined with my other comment would shave a byte off of this, but it would require a refactor at serialization.

profiling/src/profile/internal/function.rs

profiling/src/profile/internal/line.rs

profiling/src/profile/internal/mod.rs

danielsn · 2023-08-14T15:54:33Z

profiling/src/profile/internal/stack_trace.rs

+pub struct StackTrace {
+    /// The ids recorded here correspond to a Profile.location.id.
+    /// The leaf is at location_id[0].
+    pub locations: Vec<LocationId>,


This could be Box<[Location]> and save some bytes

Let's save this kind of thing for another PR, this one is big enough! But good idea.

profiling/src/profile/internal/stack_trace.rs

danielsn · 2023-08-14T16:05:46Z

profiling/src/profile/internal/value_type.rs

+
+#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
+pub struct ValueType {
+    pub r#type: StringId,


Since we're making our own struct, should we call this typ instead?

(I do prefer type personally :P )

profiling/src/profile/mod.rs

danielsn · 2023-08-14T16:09:09Z

profiling/src/profile/mod.rs

-impl<T: Sized + Hash + Eq> DedupExt<T> for FxIndexSet<T> {
-    fn dedup(&mut self, item: T) -> usize {
+impl<T: Item> Dedup<T> for FxIndexSet<T> {
+    fn dedup(&mut self, item: T) -> <T as Item>::Id {
        let (id, _) = self.insert_full(item);


Do the same assertion as intern does

I don't think it makes sense to in this case: if it already exists, that's fine. The string case is different because it can't do a simple insert_full because it has to deal with &str vs String differences.

But, it did make me realize we can move that assertion in intern to be a debug_assert.

profiling/src/profile/mod.rs

danielsn

A couple minor ⛏️ and comments but LGTM

profiling/src/profile/internal/location.rs

profiling/src/profile/internal/mod.rs

danielsn · 2023-08-15T14:00:51Z

profiling/src/profile/mod.rs

@@ -686,20 +487,22 @@ impl Profile {
        let mut labels: Vec<Label> = Vec::with_capacity(sample.labels.len());
        let mut local_root_span_id_label_offset: Option<usize> = None;
        for label in sample.labels.iter() {
+            anyhow::ensure!(
+                label.str.is_none() || label.num == 0,
+                "Invalid label: {:?}",


Do we want to explain in the message why the label is invalid?

+1 yes please! I left some suggestions for this on PR #205

I think we can combine both of the ensures into a single one:

anyhow::ensure!( label.str.is_none() || (label.num == 0 && label.num_unit.is_none()), "Invalid label: used both str and num fields: {label:?}" );

Reasoning:

If `label.str` is none, then it doesn't matter what values are held in `label.num` and `label.num_unit` else then both `label.num` and `label.num_unit` should be zero-representations

Right?

I added some tests to double-check this ^_^ They seem equivalent.

Seems good to me! The negations tripped me a bit so I had to pause a bit to mentally parse it, but the logic looks very correct!

profiling/src/profile/mod.rs

ivoanjo

Left a few notes, but overall LGTM 👍

profiling/src/profile/internal/function.rs

ivoanjo · 2023-08-15T12:59:00Z

profiling/src/profile/internal/label.rs

+    Str(StringId),
+    Num {
+        num: i64,
+        num_unit: Option<StringId>,


PHP seems to be the only profiler setting num_unit (#204). Do you think it's still useful? Otherwise it could be one more thing to get rid of :)

I'd be in favour of removing it.

If we need to, we could give an API allowing clients to specify a key, num_unit mapping, and then insert the num_unit to the pprof based on that mapping. Keeps the feature, but saves memory

Let's leave it to another PR so we can get this merged.

profiling/src/profile/internal/line.rs

ivoanjo · 2023-08-15T14:27:27Z

profiling/src/profile/internal/value_type.rs

+
+#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
+pub struct ValueType {
+    pub r#type: StringId,


(I do prefer type personally :P )

profiling/src/profile/mod.rs

ivoanjo · 2023-08-15T14:48:11Z

profiling/src/profile/mod.rs

@@ -686,20 +487,22 @@ impl Profile {
        let mut labels: Vec<Label> = Vec::with_capacity(sample.labels.len());
        let mut local_root_span_id_label_offset: Option<usize> = None;
        for label in sample.labels.iter() {
+            anyhow::ensure!(
+                label.str.is_none() || label.num == 0,
+                "Invalid label: {:?}",


+1 yes please! I left some suggestions for this on PR #205

Co-authored-by: Daniel Schwartz-Narbonne <danielsn@users.noreply.github.com>

danielsn

🚢

danielsn and others added 6 commits August 11, 2023 15:45

ValueType

bf1e7e7

Label

5eb579e

Line and Location

b02b6b7

Function

30460c5

Added Licence Headers

c264ae5

github-actions bot added the profiling Relates to the profiling* modules. label Aug 12, 2023

morrisonlevi added 2 commits August 12, 2023 15:07

add missing license header

cec5273

style: fix clippy::useless-conversion

d3f24eb

morrisonlevi changed the base branch from dsn/smaller_structs to main August 12, 2023 22:14

morrisonlevi marked this pull request as ready for review August 12, 2023 22:16

morrisonlevi requested a review from a team as a code owner August 12, 2023 22:16

sanchda reviewed Aug 13, 2023

View reviewed changes

profiling/src/profile/internal/mapping.rs Show resolved Hide resolved

sanchda reviewed Aug 13, 2023

View reviewed changes

danielsn reviewed Aug 14, 2023

View reviewed changes

morrisonlevi added 4 commits August 14, 2023 20:14

extract to_pprof_vec

009b7d1

drop unused Display for StackTraceId

9644f12

move assertion to debug_assert

883271c

avoid mut

090d818

morrisonlevi force-pushed the levi/smaller-structs branch 2 times, most recently from a068c5a to 81cbfb8 Compare August 15, 2023 10:22

extract small_non_zero_pprof_id

041231e

morrisonlevi force-pushed the levi/smaller-structs branch from 81cbfb8 to 041231e Compare August 15, 2023 10:23

morrisonlevi added 3 commits August 15, 2023 06:54

simplify imports

7c27969

use consistent copyright year

af3ac23

reduce transmute danger

49f6a1c

danielsn previously approved these changes Aug 15, 2023

View reviewed changes

ivoanjo previously approved these changes Aug 15, 2023

View reviewed changes

danielsn mentioned this pull request Aug 15, 2023

Refactor profiler to make a cleaner seperation with pprof #211

Closed

Use .into() instead of as u64

c957808

Co-authored-by: Daniel Schwartz-Narbonne <danielsn@users.noreply.github.com>

morrisonlevi dismissed stale reviews from ivoanjo and danielsn via c957808 August 15, 2023 22:00

morrisonlevi added 2 commits August 15, 2023 16:51

add Label::uses_at_most_one_of_str_and_num

2365ec6

Address CR about comments

d746266

morrisonlevi force-pushed the levi/smaller-structs branch from c5a685d to d746266 Compare August 15, 2023 23:33

add more comments

cf8fa82

danielsn approved these changes Aug 16, 2023

View reviewed changes

morrisonlevi merged commit e65f300 into main Aug 16, 2023

morrisonlevi deleted the levi/smaller-structs branch August 16, 2023 14:30

danielsn mentioned this pull request Aug 18, 2023

refactor: Move profiler internal types to the internal module #223

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(profiling): rework internal types #214

refactor(profiling): rework internal types #214

morrisonlevi commented Aug 12, 2023 •

edited

Loading

sanchda Aug 13, 2023

morrisonlevi Aug 15, 2023

sanchda Aug 13, 2023

danielsn Aug 14, 2023

morrisonlevi Aug 15, 2023

danielsn Aug 14, 2023

ivoanjo Aug 15, 2023

danielsn Aug 14, 2023

morrisonlevi Aug 15, 2023 •

edited

Loading

danielsn left a comment

danielsn Aug 15, 2023

ivoanjo Aug 15, 2023

morrisonlevi Aug 15, 2023 •

edited

Loading

morrisonlevi Aug 15, 2023

ivoanjo Aug 16, 2023

ivoanjo left a comment

ivoanjo Aug 15, 2023

danielsn Aug 15, 2023

danielsn Aug 15, 2023

morrisonlevi Aug 15, 2023

ivoanjo Aug 15, 2023

ivoanjo Aug 15, 2023

danielsn left a comment

refactor(profiling): rework internal types #214

refactor(profiling): rework internal types #214

Conversation

morrisonlevi commented Aug 12, 2023 • edited Loading

What does this PR do?

Motivation

Additional Notes

How to test the change?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morrisonlevi Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

danielsn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morrisonlevi Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivoanjo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielsn left a comment

Choose a reason for hiding this comment

morrisonlevi commented Aug 12, 2023 •

edited

Loading

morrisonlevi Aug 15, 2023 •

edited

Loading

morrisonlevi Aug 15, 2023 •

edited

Loading