Skip to content
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

Merged
merged 30 commits into from
Apr 7, 2022
Merged

Conversation

janpaul123
Copy link
Contributor

@janpaul123 janpaul123 commented Mar 25, 2022

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!

example_flamegraph --OS X Monterey, Chrome

@janpaul123 janpaul123 force-pushed the flamegraph-prototype branch 2 times, most recently from def3de5 to bdf9db0 Compare March 26, 2022 04:05
@janpaul123 janpaul123 marked this pull request as ready for review March 26, 2022 05:07
@janpaul123
Copy link
Contributor Author

cc @thomasballinger if you're interested in having a look :)

@@ -0,0 +1,270 @@
pub const NAMES: &[&str] = &[
Copy link
Contributor

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)

Copy link
Contributor Author

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();
Copy link
Contributor

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?

Copy link
Contributor Author

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])

Comment on lines +55 to +56
for flame_rect in &mut self.flame_rects {
if let FlameRectEvent::Clicked(span_index) = flame_rect.handle(cx, event) {
Copy link
Contributor

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)

Comment on lines +150 to +163
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);
Copy link
Contributor

@stevekrouse stevekrouse Mar 29, 2022

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);

Copy link
Contributor

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];
Copy link
Contributor

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)

@janpaul123 janpaul123 merged commit 0502f80 into main Apr 7, 2022
@janpaul123 janpaul123 deleted the flamegraph-prototype branch April 7, 2022 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants