Improving Performance Monitor #5895

wdanilo · 2023-03-12T16:55:54Z

Pull Request Description

This PR improves the Performance Monitor and the way we handle JavaScript events in the application.

Important Notes

In particular, this PR introduces the following changes:

The Performance Monitor has now "details" panel which can be used to display additional information. For example, the "draw call count" widget, when selected, displays all the names of symbols that caused draw calls in the given frame.
The Performance Monitor can be paused. After pausing, a selection area appears that can be dragged left and right in order to select and inspect data points from the past.
The application does not catch all JavaScript events from window. Instead, it passes them to JS visualizations, like every website should. Some JS visualizations using D3 were fixed in order not to "steal the focus".
A generic way of discovering when the mouse is not handled by EnsoGL anymore was added. Whenever the mouse hovers a div not managed by EnsoGL, the cursor will automatically revert to the system one.
Also, fixes Dragging fullscreen visualisation view also moves camera in graph editor #5075.

Screen.Recording.2023-03-17.at.06.59.22.mov

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

…sten on window and some on root.

lib/rust/ensogl/examples/profiling-run-graph/src/lib.rs

Co-authored-by: Kaz Wesley <kaz@lambdaverse.org>

farmaazon · 2023-03-17T10:41:18Z

app/gui/suggestion-database/src/mock.rs

-// === Exports ===
-// ===============
-
-// Re-exported here so it can be referenced by macro-generated code.


Why removed this comment? Isn't it useful?

It causes enso-formatter to fail on this file.

farmaazon · 2023-03-17T10:46:15Z

app/gui/view/graph-editor/src/component/visualization/foreign/java_script/instance.rs

@@ -247,6 +246,11 @@ impl InstanceModel {
    fn set_layer(&self, layer: Layer) {
        layer.apply_for_html_component(&self.scene, &self.root_node)
    }
+
+    fn set_active(&self, active: bool) {
+        let val = if active { "all" } else { "none " };


val -> attribute as it says a bit more.

Is the space after "none" necessary?

farmaazon · 2023-03-17T10:49:34Z

lib/rust/ensogl/core/src/application.rs

@@ -105,13 +105,13 @@ impl Application {
        let data = &self.inner;
        let network = self.frp.network();
        enso_frp::extend! { network
+            eval self.display.default_scene.frp.focused ((t) data.show_system_cursor(!t));


show_system_cursor name is a bit misleading as it sometimes hides it. set_system_cursor maybe?

I'd disagree on that one. show_system_cursor(false) is more understandable to me than set_system_cursor(false). However, I'd love to chat about it more if you want :)

farmaazon · 2023-03-17T10:53:19Z

lib/rust/ensogl/core/src/control/io/mouse.rs

        }

        impl MouseManager {
-            /// Constructor.
+            /// Constructor. See docs of `new_separated` to learn more.


Suggested change

/// Constructor. See docs of `new_separated` to learn more.

/// Constructor. See docs of [`Self::new_separated`] to learn more.

Or new_separated.

You can check in IntelliJ if the doc links work by changing to rich mode, clicking on the links and check if proper docs are displayed.

The idea with checking it in IntelliJ is amazing, but somehow it does not work on my end. Have seen it before?

farmaazon · 2023-03-17T10:57:03Z

lib/rust/ensogl/core/src/control/io/mouse.rs

+            /// 1. A DOM object to set resize observer on. This object should cover the entire screen.
+            /// Since EnsoGL's scene origin is positioned in the left-bottom corner, the size of
+            /// the DOM object is used to translate mouse coordinates from HTML to the EnsoGL space.
+            ///
+            /// 2. A DOM object to set the 'mousedown', 'mousewheel', and 'mouseleave' listeners on.
+            /// In most cases, this should be the canvas used by EnsoGL. Alternatively, you can set
+            /// this argument to the window object if you want EnsoGL to capture all events, even if
+            /// it is placed behind another DOM element.
+            ///
+            /// 3. A DOM object to set the 'mouseup' and 'mousemove' listeners on. In most cases,
+            /// this should be the window object. It is common for the element drag action to be
+            /// initiated by a 'mousedown' event on one element and finished by a 'mouseup' event
+            /// on another element. Handling these events globally covers such situations.
+            pub fn new_separated(


If those three differ "in most cases", then perhaps this should be a main constructor, and new should be called differently or just removed to not encourage people to make things unusual.

farmaazon · 2023-03-17T11:21:58Z

lib/rust/ensogl/core/src/debug/monitor.rs

+// =================================================================================================
+// === Samplers ====================================================================================
+// =================================================================================================


This "Long heading" even more suggests splitting the file into submodules. We should not have modules requiring three tiers of headings.

It was like that (I know this is not an explanation). Im refactoring it rn.

farmaazon · 2023-03-17T12:30:45Z

lib/rust/frp/src/io/js.rs

@@ -91,82 +91,42 @@ fn event_listener_options() -> enso_web::AddEventListenerOptions {
 /// `pass_to_dom` event will be emitted.
 ///
 /// To make this class manage js event, you should wrap the closure passed as event listener using
-/// `make_event_handler` function.
+/// `handler` function.


Update the docs. We do not have "pass_to_dom" anymore.

In fact, I'm not sure if we need this function anymore. Before, we wanted to keep JsEvent to be able to call "prevent_default" if nobody allowed passing it to DOM. Now, I don't see any scenario where we need global access to currently processed event.

I'm not sure here. I like this structure, as it wraps all JS events in FRP. I'd leave it this way, as it should not affect performance (we are doing that only once when registering bindings) and it gives nice FRP access to all JS events. Seems handy. Docs updated.

farmaazon · 2023-03-17T12:36:17Z

app/gui/view/graph-editor/src/builtin/visualization/java_script/histogram.js

+    /** Removing `pointer-events` handling from brush element, as we want it to be inherited. D3 inserts
+     * `pointer-events: all` in the brush element and some of its children on brush creation and after brushing ends. */


Is there any documentation to link, or is it just your discovery? You can add this little info to the comment.

It's my discovery, but you are right, I'm adding a better description there

farmaazon · 2023-03-17T12:36:53Z

build/build/src/project/wasm.rs

+            let (watch_cmd_name, mut watch_cmd_opts) = match std::env::var("USE_CARGO_WATCH_PLUS") {
+                Ok(_) => ("watch-plus", vec!["--why"]),
+                Err(_) => ("watch", vec![]),
+            };
+            watch_cmd_opts.push("--ignore");
+            watch_cmd_opts.push("README.md");
+
            watch_cmd
                .kill_on_drop(true)
                .current_dir(&context.repo_root)
-                .arg("watch")
-                .args(["--ignore", "README.md"])
+                .arg(watch_cmd_name)
+                .args(watch_cmd_opts)


Add documentation of this feature somewhere.

Added to contributing guide.

farmaazon · 2023-03-17T12:39:56Z

lib/rust/ensogl/core/src/display/world.rs

    fn run_stats(&self, time: Duration) {
-        let previous_frame_stats = self.stats.begin_frame(time);
-        if let Some(stats) = previous_frame_stats {
-            self.on.prev_frame_stats.run_all(&stats);
+        self.stats.calculate_prev_frame_fps(time);
+        {
+            let stats_borrowed = self.stats.borrow();
+            self.on.prev_frame_stats.run_all(&stats_borrowed.stats_data);
        }
+        self.stats.reset_per_frame_statistics();
    }


I know it's from older code, but I don't quite get what "running stats" means in this context. It is about displaying them?

where is "running stats" there? run_all refers to running all registered callbacks.

In the method name. But indeed, I missed that those inside are callbacks.

kazcw · 2023-03-17T15:40:12Z

lib/rust/ensogl/core/src/debug/stats.rs

    }

    /// Ends tracking data for the current animation frame.
    /// Also, calculates the `frame_time` and `wasm_memory_usage` stats.
    pub fn end_frame(&self) {
        self.rc.borrow_mut().end_frame();
    }
+
+    /// Register a new draw call for the given symbol.
+    pub fn new_draw_call(&self, symbol_id: SymbolId) {


new_ is so strongly associated with constructors

kazcw · 2023-03-17T15:42:19Z

lib/rust/ensogl/core/src/debug/monitor.rs

+const PADDING_LEFT: f64 = 8.0;
+const PADDING_TOP: f64 = 8.0;
+const FONTS: &str =
+    "\"SF Pro Text\",\"SF Pro Icons\",\"Helvetica Neue\",\"Helvetica\",\"Arial\",sans-serif";


r#"Raw quotes"# would avoid the backslashes here.

kazcw · 2023-03-17T15:49:27Z

lib/rust/ensogl/core/src/debug/monitor.rs

+    details:          web::HtmlDivElement,
+    selection_offset: Cell<f64>,
+    plot_area:        MainArea,
+    control_button:   ControlButton,
 }

 impl Dom {
    /// Constructor.
    #[allow(clippy::new_without_default)]


Unnecessary lint.

Rust should tell about it :( Thanks for catching it!

kazcw · 2023-03-17T15:50:38Z

lib/rust/ensogl/core/src/debug/monitor.rs

+        let on_pause_press = Rc::new(on_pause_press);
+        let on_play_press = Rc::new(on_play_press);
+        let on_main_press = Rc::new(on_main_press);
+        Self { rc, on_pause_press, on_play_press, on_main_press }
    }
 }


 impl DomData {
    /// Constructor.
    #[allow(clippy::new_without_default)]


Unnecessary lint.

Rust should tell about it :( Thanks for catching it!

kazcw · 2023-03-17T15:52:40Z

lib/rust/ensogl/core/src/debug/monitor.rs

+    pub fn move_selection(&self, offset: f64) -> usize {
+        let start_x = self.config.plot_x();
+        let end_x = self.config.plot_right_x();
+        let width = end_x - start_x - 1.0;


Magic 1.0. Is it the selection width?

kazcw · 2023-03-17T16:07:49Z

lib/rust/ensogl/core/src/debug/monitor.rs

+        let selection_border = config.plot_selection_border;
+        let selection_width = config.plot_selection_width + 2.0 * selection_border;
+        let selection_left =
+            config.plot_right_x() - 0.5 - config.plot_selection_width / 2.0 - selection_border;


What's 0.5 here?

It's centering of selection. I refactored the code to be more obvious and moved it to

let selection_center = (config.plot_selection_width + 1.0) / 2.0;

Is it ok now?

kazcw · 2023-03-17T16:10:21Z

lib/rust/ensogl/core/src/debug/monitor.rs

+        let pause_icon = "<svg width=\"16\" height=\"16\">
+           <circle cx=\"8\" cy=\"8\" r=\"8\" fill=\"#00000020\" />
+           <rect x=\"5\" y=\"4\" width=\"2\" height=\"8\" style=\"fill:#00000080;\" />
+           <rect x=\"9\" y=\"4\" width=\"2\" height=\"8\" style=\"fill:#00000080;\" />
+        </svg>";
+
+        let play_icon = "<svg width=\"16\" height=\"16\">
+           <circle cx=\"8\" cy=\"8\" r=\"8\" fill=\"#00000020\" />
+           <polygon points=\"6,4 12,8 6,12\" style=\"fill:#00000080;\" />
+        </svg>";


Raw quotes would be easier to read here.

kazcw · 2023-03-17T16:17:33Z

lib/rust/ensogl/core/src/debug/monitor.rs

+        let panel_index = ((position.y as f64
+            - PADDING_TOP
+            - self.user_config.outer_margin
+            - self.user_config.margin / 2.0)
+            / (self.user_config.margin + self.user_config.panel_height))
+            .floor() as usize;
+        let panel_index = panel_index.max(0).min(self.panels.len() - 1);


.max(0) should be done before converting to an unsigned type

Oh, good point!

kazcw · 2023-03-17T16:25:37Z

lib/rust/ensogl/core/src/debug/monitor.rs

+        }
+    }
+
+    fn set_focus_sample(&mut self, index: Option<usize>) {


It seems like some additional logic would be necessary to handle index=None, but this function is never used that way. Maybe index: usize?

Thanks for catching it!

kazcw · 2023-03-17T16:36:09Z

lib/rust/ensogl/core/src/debug/monitor.rs

    fn shift_plot_area_left(&mut self, dom: &Dom) {
        let width = self.width;
        let height = self.height;
        let shift = -(self.config.plot_step_size);
-        dom.context
+        dom.plot_area
+            .plot_context
            .draw_image_with_html_canvas_element_and_sw_and_sh_and_dx_and_dy_and_dw_and_dh(


What a name, web_sys 😆

Yeah, it's hilarious :D

kazcw · 2023-03-18T03:24:24Z

QA ✔️ 🎉

Details panel shows symbol draw counts
PM pause, inspect historic counts, and resume
Mouse works correctly in D3 visualizations
Dragging fullscreen visualisation view also moves camera in graph editor #5075 fixed

farmaazon · 2023-03-20T15:43:13Z

QA Report

Navigating a full screen Geo Map still seems to affect the scene underneath.

Also, I've found several issues with visualization, but they turned out to be present also on develop. Here I put list for reference:

Scatter plot points seem to be not visible after drag
Scatter plot Zooming and fitting to all is not animated
Histogram fitting to all is not animated
Scrolling JSON visualization with mouse wheel does not work.

wdanilo · 2023-03-21T08:17:17Z

@farmaazon as these are not regressions, I'll merge it still. The thing that the full-screen visualizations work is an accidental fix here nad TBH I don't know why it's fixed, I was not digging into it. So this issue with geo map needs investigation and digging deeper.

wdanilo added 15 commits February 19, 2023 08:14

Implementing perf monitor hover

1ee4871

Refactoring

304010f

Adding details view to performance monitor

b4941f1

Refactoring

ccc8ab9

Merge branch 'develop' into wip/wdanilo/render-stats

5156ef6

Refactoring

60fd979

Refactoring

3afcc6a

Refactoring

e78a9ba

Improving error handling

5485aa4

Adding pointer-events handling to JS visualization engine

dcc49b4

Fixing d3 visualizations to not capture mouse events globally.

f559e97

Cleaning JS event handling in FRP

80cc391

refactoring

e00eb49

Differentation of mouse event behavior targets. Making some events li…

309aab6

…sten on window and some on root.

Linting

93507a0

kazcw reviewed Mar 16, 2023

View reviewed changes

lib/rust/ensogl/examples/profiling-run-graph/src/lib.rs Outdated Show resolved Hide resolved

wdanilo and others added 10 commits March 16, 2023 19:51

Update lib/rust/ensogl/examples/profiling-run-graph/src/lib.rs

aa2194a

Co-authored-by: Kaz Wesley <kaz@lambdaverse.org>

Applying diff from Kaz

e6334bb

Refactoring

3c9167c

Linting

65ff1f2

Refactoring

987b57a

Merge branch 'develop' into wip/wdanilo/render-stats

371dd02

Linting

e7548d2

Removing accidentaly generated file

68d3b19

Removing scala parser - brought back by git merge

3b282ea

Fixing cursor handling

9aa0c70

wdanilo marked this pull request as ready for review March 17, 2023 05:47

wdanilo requested review from MichaelMauderer, farmaazon and mwu-tow as code owners March 17, 2023 05:47

wdanilo changed the title ~~Wip/wdanilo/render stats~~ Improving Performance Monitor Mar 17, 2023

Linting and formatting

8c188f3

wdanilo requested a review from JaroslavTulach as a code owner March 17, 2023 06:01

farmaazon approved these changes Mar 17, 2023

View reviewed changes

kazcw approved these changes Mar 17, 2023

View reviewed changes

wdanilo added 2 commits March 17, 2023 22:01

Applying review

3a09437

Merge branch 'develop' into wip/wdanilo/render-stats

bee754f

enso-bot bot mentioned this pull request Mar 20, 2023

Vector_Editor widget under the current node #5946

Closed

Merge branch 'develop' into wip/wdanilo/render-stats

f69b75a

wdanilo merged commit abb0b44 into develop Mar 21, 2023

wdanilo deleted the wip/wdanilo/render-stats branch March 21, 2023 08:17

	/// Constructor. See docs of `new_separated` to learn more.
	/// Constructor. See docs of [`Self::new_separated`] to learn more.

		/** Removing `pointer-events` handling from brush element, as we want it to be inherited. D3 inserts
		* `pointer-events: all` in the brush element and some of its children on brush creation and after brushing ends. */

Improving Performance Monitor #5895

Improving Performance Monitor #5895

Conversation

wdanilo commented Mar 12, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazcw commented Mar 18, 2023

QA ✔️ 🎉

farmaazon commented Mar 20, 2023

QA Report

wdanilo commented Mar 21, 2023

wdanilo commented Mar 12, 2023 •

edited

Loading