-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Flamegraph prototype + text improvements #176
Conversation
def3de5
to
bdf9db0
Compare
cc @thomasballinger if you're interested in having a look :) |
@@ -0,0 +1,270 @@ | |||
pub const NAMES: &[&str] = &[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we use a JSON representation and then parse it, given that we already have to be able to deal with their standard JSON representation? (Seems like work to have to maintain this file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I'm not sure what their internal format is — whether it is that JSON or if they first transform it to something else internally. Once I've figured that out I'll update this fixture to what makes most sense.
#[cfg(test)] | ||
pub fn new_test() -> Self { | ||
let mut cx = Self::new(TypeId::of::<()>()); | ||
cx.load_fonts(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any assertions here, so this test just makes sure that no error is thrown? If so, is there to be explicit about the fact that that's what's being tested here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah this is not a test; this is a helper for use within tests. I'll add a comment! (Tests would be marked with #[test]
)
for flame_rect in &mut self.flame_rects { | ||
if let FlameRectEvent::Clicked(span_index) = flame_rect.handle(cx, event) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I demoed this to someone recently (I forget whom), they asked "wait does this handle
function run on every event Zaplib detects? Every mousemove? How is that not super slow and inefficient?"
(I share this question)
self.window.begin_window(cx); | ||
self.pass.begin_pass(cx, Vec4::color("300")); | ||
self.main_view.begin_view(cx, LayoutSize::FILL); | ||
cx.begin_padding_box(Padding::top(30.)); | ||
|
||
self.flame_rects.resize_with(self.spans.len(), Default::default); | ||
for (span_index, span) in self.spans.iter().enumerate() { | ||
self.flame_rects[span_index].draw(cx, span_index, span, self.zoom_pan); | ||
} | ||
|
||
cx.end_padding_box(); | ||
self.main_view.end_view(cx); | ||
self.pass.end_pass(cx); | ||
self.window.end_window(cx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be nice to have indentation but this isn't great:
self.window.begin_window(cx);
{
self.pass.begin_pass(cx, Vec4::color("300"));
{
self.main_view.begin_view(cx, LayoutSize::FILL);
{
cx.begin_padding_box(Padding::top(30.));
{
self.flame_rects.resize_with(self.spans.len(), Default::default);
for (span_index, span) in self.spans.iter().enumerate() {
self.flame_rects[span_index].draw(cx, span_index, span, self.zoom_pan);
}
};
cx.end_padding_box();
};
self.main_view.end_view(cx);
};
self.pass.end_pass(cx);
};
self.window.end_window(cx);
I think this is valid syntax but cargo fmt
doesn't like this:
self.window.begin_window(cx); {
self.pass.begin_pass(cx, Vec4::color("300")); {
self.main_view.begin_view(cx, LayoutSize::FILL); {
cx.begin_padding_box(Padding::top(30.)); {
self.flame_rects.resize_with(self.spans.len(), Default::default);
for (span_index, span) in self.spans.iter().enumerate() {
self.flame_rects[span_index].draw(cx, span_index, span, self.zoom_pan);
}
}; cx.end_padding_box();
}; self.main_view.end_view(cx);
}; self.pass.end_pass(cx);
}; self.window.end_window(cx);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regardless we'll need to explain what's happening here and how you can imagine it like JSX but with imperative syntax (to get around the borrow checker)
fn handle(&mut self, cx: &mut Cx, event: &mut Event) { | ||
for flame_rect in &mut self.flame_rects { | ||
if let FlameRectEvent::Clicked(span_index) = flame_rect.handle(cx, event) { | ||
let span = &self.spans[span_index]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to explain here why we can pass a pointer to the span down into the FlameRect component but not back up (we'd need to copy it or use a ref counter, which is similar to a weak ref in JS, because of lifecycle issues which always happens for recursive things like this)
1. Not taking into account the size of the ellipsis itself 2. Gradually going from no ellipsis to “.” then “..” then “..." 3. Not hiding the ellipsis itself if there’s no room for it
To avoid unnecessary allocations.
Fully zooming/panning so it spans the entire container.
They’re very common in graphics programming.
This does add a bunch of dependencies, which is unfortunate. I added some thoughts on how to optimize this further in the future.
I removed the unnecessary `Wrapping::Line` option (per comment). I added a bunch of unit tests. For this I also had to add a `Cx::new_test`.
bdf9db0
to
38c6325
Compare
Adds a basic "flamegraph prototype" which is eventually to be integrated with Pyroscope; but a bunch more work has to be done for that (#177).
This also refactors text rendering a bunch so that we get ellipsis truncation even when using custom layouting. And adds some unit tests to that.
Best viewed by individual commits!