Conversation
blp
added
performance
rust
There was a problem hiding this comment.
these look like milliseconds to me
why not serialize the nanoseconds?
There was a problem hiding this comment.
Oops, you are right. I will change the name.
The serialized version must match the specification at https://github.com/firefox-devtools/profiler/blob/main/src/types/profile.ts.
| } | ||
|
|
||
| /// Calls `f` and records the span. Returns whatever `f` returned. | ||
| pub fn in_scope<F, T>(self, f: F) -> T |
There was a problem hiding this comment.
why in_scope? I would call this record_span or record_span_of_function
There was a problem hiding this comment.
This is meant to imitate the tracing::Span API at https://docs.rs/tracing/latest/tracing/struct.Span.html#method.in_scope
| /// To start a capture, use [CaptureOptions::start], | ||
| /// [CaptureOptions::blocking_start], or [CaptureOptions::try_start]. | ||
| /// | ||
| /// Only one `Capture` may exist at one time. |
There was a problem hiding this comment.
Only one anywhere in the universe. If there are two, we must restart from the Big Bang, and we will all have to go get our PhDs again, and then we will have to do postdocs, too, even if we skipped them this time around.
| profile.meta.os_cpu = os_cpu; | ||
| } | ||
| profile.meta.marker_schema.push(json!({ | ||
| "name": "FelderaMarker", |
There was a problem hiding this comment.
It's an arbitrary name for which we only need uniqueness. I think it's reasonable to use Feldera in the name for that reason.
|
|
||
| #[derive(Debug, Serialize, Deserialize)] | ||
| #[serde(rename_all = "camelCase")] | ||
| struct Profile { |
There was a problem hiding this comment.
Are these structs reverse-engineered from some JSON format of the profile? Or maybe they are documented?
There was a problem hiding this comment.
They are documented (sort of) at https://github.com/firefox-devtools/profiler/blob/main/src/types/profile.ts and https://github.com/firefox-devtools/profiler/blob/main/src/types/markers.ts
| } | ||
| } | ||
|
|
||
| thread_local! { |
There was a problem hiding this comment.
Since this is a global variable, it would be nice to document it.
|
|
||
| thread_local! { | ||
| static QUEUE: Arc<Queue> = Queue::new(); | ||
| } |
There was a problem hiding this comment.
But if you add more comments, the file won't have exactly 1000 lines anymore, so don't do it.
| @@ -0,0 +1 @@ | |||
| edition = "2024" No newline at end of file | |||
There was a problem hiding this comment.
can we just make this the workspace default? by moving this one level up?
There was a problem hiding this comment.
Yes, if we do one of the following:
- Migrate all our crates to 2024 edition.
- Change the crates that are 2021 edition to instead explicitly say so.
The former is probably better, but also more work.
The crates this affects are feldera-macros, iceberg, ir, pipeline-manager, rest-api, and storage-test-compat.
There was a problem hiding this comment.
The former is probably better, but also more work.
I thought this is just about formatting, does it involve more than just cargo fmt with this edition set?
There was a problem hiding this comment.
Oh, hmm, it seems like all our crates format OK with 2024 edition. They don't format OK with 2021 edition because then cargo fmt chokes on if let ... && let ....
I'll make that change.
There was a problem hiding this comment.
That sounds like a separate PR
|
note that because we haven't released v288 for the other crates, ideally we have wait until we release v289 to merge it to not break the release (and adjust the cargo version for it to 289) -- I fear otherwise it might complain during release trying to release someting that's already published |
I'll wait to merge this then. No worries. |
| } | ||
| } | ||
|
|
||
| /// Enables or disables recording the amount of memory in use (the resident |
There was a problem hiding this comment.
This PR is not just a crate move: it adds new runtime behavior by recording and emitting the memory graph data into annotated profiles, but I do not see any unit or integration coverage for that new path. Per our test rule, behavior changes need automated coverage. Please add a focused test around the new annotation logic -- e.g. a small synthetic profile/assertion that Annotations::apply() emits the expected memory track when RSS samples are present, plus one no-memory case so this does not silently regress later.
Signed-off-by: Ben Pfaff <blp@feldera.com>
Signed-off-by: Ben Pfaff <blp@feldera.com>
Signed-off-by: Ben Pfaff <blp@feldera.com>
Signed-off-by: Ben Pfaff <blp@feldera.com>
We don't define the format; it comes from the Firefox profiler. Signed-off-by: Ben Pfaff <blp@feldera.com>
Signed-off-by: feldera-bot <feldera-bot@feldera.com>
5 participants
This moves the samply code into a new crate and then adds support for a memory graph (part of #6032).