Rendering improvement/debugging support (#6019)

Some small improvements relating to rendering:

- Add a debug option: `-debug.pixel-read-period`. This can be used to measure the performance impact of checking the pointer location on different hardware. [On my development box, it makes no difference to performance.] (Closes #5490).
- Unbind pixel pack buffers after each use. This is recommended practice. It has no performance impact on my machine, and allows SpectorJS to run (`-debug.enable-spector`). (Closes #5941).

Also, simplify the profiling CLI: the `profile.load-profile` and `profile.save-profile` options have been renamed to `profile.load`/`profile.save`; `profile.save` now has a default filename, so you can capture a profile at any time in Electron with Ctrl+Alt+P and it will be written to `profile.json`.

app/gui/config/build.rs

@@ -1,8 +1,16 @@
 const CONFIG_PATH: &str = "../config.yaml";
 
+const JSON_CONFIG: &[&str] = &[
+    "../../../lib/rust/ensogl/pack/js/src/runner/config.json",
+    "../../../app/ide-desktop/lib/content-config/src/config.json",
+];
+
 fn main() {
     println!("cargo:rerun-if-changed={CONFIG_PATH}");
     println!("cargo:rerun-if-changed=build.rs");
+    for path in JSON_CONFIG {
+        println!("cargo:rerun-if-changed={path}");
+    }
 
     config_reader::generate_config_module_from_yaml(CONFIG_PATH);
 }

app/gui/src/ide/initializer.rs

@@ -70,6 +70,14 @@ impl Initializer {
 
         ensogl_text_msdf::initialized().await;
         let ensogl_app = ensogl::application::Application::new(self.config.dom_parent_id());
+        let pixel_read_period =
+            enso_config::ARGS.groups.debug.options.pixel_read_period.value.parse();
+        match pixel_read_period {
+            Ok(read_period) => ensogl_app.display.set_pixel_read_period(read_period),
+            Err(e) => {
+                error!("Invalid pixel read period argument provided: `{e}`.");
+            }
+        }
         register_views(&ensogl_app);
         let view = ensogl_app.new_view::<ide_view::root::View>();
 

app/ide-desktop/lib/client/src/config.ts

@@ -256,7 +256,7 @@ export const CONFIG = contentConfig.OPTIONS.merge(
 
             profile: new contentConfig.Group({
                 options: {
-                    loadProfile: new contentConfig.Option<string[]>({
+                    load: new contentConfig.Option<string[]>({
                         passToWebApplication: false,
                         value: [],
                         description:
@@ -264,13 +264,13 @@ export const CONFIG = contentConfig.OPTIONS.merge(
                             `the 'profiling-run-graph' entry point.`,
                         primary: false,
                     }),
-                    saveProfile: new contentConfig.Option({
+                    save: new contentConfig.Option({
                         passToWebApplication: false,
-                        value: '',
+                        value: 'profile.json',
                         description:
                             `Record a performance profile and save it to a file. To view the ` +
                             `results, use the 'profiling-run-graph' entry point, such as ` +
-                            `'enso -startup.entry=profiling-run-graph -profile.load-profile=profile.json'.`,
+                            `'enso -startup.entry=profiling-run-graph -profile.load=profile.json'.`,
                         primary: false,
                     }),
                 },

app/ide-desktop/lib/client/src/index.ts

@@ -242,7 +242,7 @@ class App {
         electron.ipcMain.on(ipc.Channel.error, (_event, data) => {
             logger.error(`IPC error: ${JSON.stringify(data)}`)
         })
-        const argProfiles = this.args.groups.profile.options.loadProfile.value
+        const argProfiles = this.args.groups.profile.options.load.value
         const profilePromises: Promise<string>[] = argProfiles.map((path: string) =>
             fs.readFile(path, 'utf8')
         )
@@ -252,7 +252,7 @@ class App {
                 event.reply('profiles-loaded', profiles)
             })
         })
-        const profileOutPath = this.args.groups.profile.options.saveProfile.value
+        const profileOutPath = this.args.groups.profile.options.save.value
         if (profileOutPath) {
             electron.ipcMain.on(ipc.Channel.saveProfile, (_event, data: string) => {
                 fsSync.writeFileSync(profileOutPath, data)

app/ide-desktop/lib/content-config/src/config.json

@@ -51,7 +51,6 @@
         }
       }
     },
-
     "engine": {
       "description": "Options that control the Enso Engine, the data processing backend.",
       "options": {
@@ -95,7 +94,6 @@
         }
       }
     },
-
     "style": {
       "description": "The available visual and tactile configurations of the application.",
       "options": {
@@ -105,7 +103,6 @@
         }
       }
     },
-
     "featurePreview": {
       "description": "Options that enable experimental features that are not yet stable.",
       "options": {
@@ -121,7 +118,6 @@
         }
       }
     },
-
     "authentication": {
       "description": "Options to manage application authentication properties.",
       "options": {
@@ -133,7 +129,6 @@
       },
       "primary": false
     },
-
     "profile": {
       "description": "Options for diagnosing application performance problems.",
       "options": {

app/ide-desktop/lib/content/esbuild-config.ts

@@ -141,12 +141,6 @@ export function bundlerOptions(args: Arguments) {
         incremental: trueBoolean,
         color: trueBoolean,
         logOverride: {
-            // Happens in ScalaJS-generated parser (scala-parser.js):
-            //    6 │   "fileLevelThis": this
-            'this-is-undefined-in-esm': 'silent',
-            // Happens in ScalaJS-generated parser (scala-parser.js):
-            // 1553 │   } else if ((a === (-0))) {
-            'equals-negative-zero': 'silent',
             // Happens in Emscripten-generated MSDF (msdfgen_wasm.js):
             //    1 │ ...typeof module!=="undefined"){module["exports"]=Module}process["o...
             'commonjs-variable-in-esm': 'silent',

build/build/paths.yaml

@@ -87,9 +87,7 @@
       dist/: # Here ensogl-pack outputs its artifacts
       linked-dist/: # Either symlink to dist or to the gui artifacts.
     generated-java/:
-    parser-upload/:
     test-results/:
-    scala-parser.js:
   test/:
     Benchmarks/:
   tools/:

docs/profiler/profiling.md

@@ -135,9 +135,9 @@ procedure to collect data is:
   ./run ide build --profiling-level=detail
   ```
 - To profile in Electron:
-  - Start Enso with the `save-profile` option, e.g.:
+  - Start Enso with the `profile.save` option, e.g.:
     ```shell
-    ./distribution/bin/enso --save-profile=your_profile.json
+    ./distribution/bin/enso --profile.save=your_profile.json
     ```
   - Perform any activity the profiler should record (the profiler is always
     running).
@@ -164,13 +164,13 @@ without user-interaction. It can be run as follows:
 # First build with full profiling information.
 ./run ide build --profiling-level=debug
 # Now run the generated binary in batch mode.
-./distribution/bin/enso --entry-point profile --workflow create_node --save-profile create_node.json
+./distribution/bin/enso --startup.entry=profile --profile.workflow=create_node --profile.save=create_node.json
 ```
 
 The `profile` entry point selects batch mode; when it is used, the
-`save-profile` and `workflow` arguments are mandatory. `save-profile` specifies
-an output filename; `workflow` selects from a list of defined batch-mode
-workflows to measure. Supported workflows include:
+`profile.workflow` argument is mandatory. `profile.save` specifies an output
+filename; `profile.workflow` selects from a list of defined batch-mode workflows
+to measure. Supported workflows include:
 
 - `new_project`: This workflow includes starting the application, opening an
   empty project, and closing the application.
@@ -218,7 +218,7 @@ https://www.cs.middlebury.edu/~candrews/showcase/infovis_techniques_s16/icicle_p
 Invocation:
 
 ```shell
-./distribution/bin/enso --entry-point=profiling-run-graph --load-profile=profile.json
+./distribution/bin/enso --startup.entry=profiling-run-graph --profile.load=profile.json
 ```
 
 It takes as input the profiling data representing a single run

lib/rust/ensogl/core/src/display/render/passes/pixel_read.rs

@@ -51,15 +51,15 @@ impl<T: JsTypedArrayItem> PixelReadPassData<T> {
 #[derive(Derivative, Clone)]
 #[derivative(Debug)]
 pub struct PixelReadPass<T: JsTypedArrayItem> {
-    data:          Option<PixelReadPassData<T>>,
-    sync:          Option<WebGlSync>,
-    position:      Uniform<Vector2<i32>>,
-    threshold:     usize,
-    to_next_read:  usize,
+    data:            Option<PixelReadPassData<T>>,
+    sync:            Option<WebGlSync>,
+    position:        Uniform<Vector2<i32>>,
+    threshold:       Rc<Cell<usize>>,
+    since_last_read: usize,
     #[derivative(Debug = "ignore")]
-    callback:      Option<Rc<dyn Fn(Vec<T>)>>,
+    callback:        Option<Rc<dyn Fn(Vec<T>)>>,
     #[derivative(Debug = "ignore")]
-    sync_callback: Option<Rc<dyn Fn()>>,
+    sync_callback:   Option<Rc<dyn Fn()>>,
 }
 
 impl<T: JsTypedArrayItem> PixelReadPass<T> {
@@ -70,9 +70,9 @@ impl<T: JsTypedArrayItem> PixelReadPass<T> {
         let position = position.clone_ref();
         let callback = default();
         let sync_callback = default();
-        let threshold = 0;
-        let to_next_read = 0;
-        Self { data, sync, position, threshold, to_next_read, callback, sync_callback }
+        let threshold = default();
+        let since_last_read = 0;
+        Self { data, sync, position, threshold, since_last_read, callback, sync_callback }
     }
 
     /// Sets a callback which will be evaluated after a successful pixel read action.
@@ -91,11 +91,11 @@ impl<T: JsTypedArrayItem> PixelReadPass<T> {
         self.sync_callback = Some(Rc::new(f));
     }
 
-    /// Sets a threshold of how often the pass should be run. Threshold of 0 means that it will be
-    /// run every time. Threshold of N means that it will be only run every N-th call to the `run`
-    /// function.
-    pub fn set_threshold(&mut self, threshold: usize) {
-        self.threshold = threshold;
+    /// Returns a reference that can be used to set the threshold of how often the pass should be
+    /// run. Threshold of 0 means that it will be run every time. Threshold of N means that it will
+    /// be only run every N-th call to the `run` function.
+    pub fn get_threshold(&mut self) -> Rc<Cell<usize>> {
+        self.threshold.clone()
     }
 
     fn init_if_fresh(&mut self, context: &Context, variables: &UniformScope) {
@@ -106,9 +106,10 @@ impl<T: JsTypedArrayItem> PixelReadPass<T> {
             let usage = Context::DYNAMIC_READ;
             context.bind_buffer(*target, Some(&buffer));
             context.buffer_data_with_opt_array_buffer(*target, Some(&js_array.buffer()), *usage);
+            context.bind_buffer(*target, None);
 
             let texture = match variables.get("pass_id").unwrap() {
-                uniform::AnyUniform::Texture(t) => t,
+                AnyUniform::Texture(t) => t,
                 _ => panic!("Pass internal error. Unmatched types."),
             };
             let format = texture.get_format();
@@ -138,6 +139,7 @@ impl<T: JsTypedArrayItem> PixelReadPass<T> {
         }
     }
 
+    #[profile(Detail)]
     fn run_not_synced(&mut self, context: &Context) {
         let data = self.data.as_ref().unwrap();
         let position = self.position.get();
@@ -151,6 +153,7 @@ impl<T: JsTypedArrayItem> PixelReadPass<T> {
         context
             .read_pixels_with_i32(position.x, position.y, width, height, format, typ, offset)
             .unwrap();
+        context.bind_buffer(*Context::PIXEL_PACK_BUFFER, None);
         let condition = Context::SYNC_GPU_COMMANDS_COMPLETE;
         let flags = 0;
         let sync = context.fence_sync(*condition, flags).unwrap();
@@ -158,6 +161,7 @@ impl<T: JsTypedArrayItem> PixelReadPass<T> {
         context.flush();
     }
 
+    #[profile(Detail)]
     fn check_and_handle_sync(&mut self, context: &Context, sync: &WebGlSync) {
         let data = self.data.as_ref().unwrap();
         let status = context.get_sync_parameter(sync, *Context::SYNC_STATUS);
@@ -173,6 +177,7 @@ impl<T: JsTypedArrayItem> PixelReadPass<T> {
                 offset,
                 buffer_view,
             );
+            context.bind_buffer(*Context::PIXEL_PACK_BUFFER, None);
             if let Some(f) = &self.callback {
                 f(data.js_array.to_vec());
             }
@@ -182,10 +187,10 @@ impl<T: JsTypedArrayItem> PixelReadPass<T> {
 
 impl<T: JsTypedArrayItem> pass::Definition for PixelReadPass<T> {
     fn run(&mut self, instance: &pass::Instance, update_status: UpdateStatus) {
-        if self.to_next_read > 0 {
-            self.to_next_read -= 1;
+        if self.since_last_read < self.threshold.get() {
+            self.since_last_read += 1;
         } else {
-            self.to_next_read = self.threshold;
+            self.since_last_read = 0;
             self.init_if_fresh(&instance.context, &instance.variables);
             if let Some(sync) = self.sync.clone() {
                 self.check_and_handle_sync(&instance.context, &sync);

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

@@ -417,6 +417,7 @@ pub struct WorldData {
     update_themes_handle: callback::Handle,
     garbage_collector: garbage::Collector,
     emit_measurements_handle: Rc<RefCell<Option<callback::Handle>>>,
+    pixel_read_pass_threshold: Rc<RefCell<Weak<Cell<usize>>>>,
 }
 
 impl WorldData {
@@ -441,6 +442,7 @@ impl WorldData {
         let update_themes_handle = on.before_frame.add(f_!(themes.update()));
         let emit_measurements_handle = default();
         SCENE.with_borrow_mut(|t| *t = Some(default_scene.clone_ref()));
+        let pixel_read_pass_threshold = default();
 
         Self {
             frp,
@@ -456,6 +458,7 @@ impl WorldData {
             update_themes_handle,
             garbage_collector,
             emit_measurements_handle,
+            pixel_read_pass_threshold,
         }
         .init()
     }
@@ -523,8 +526,7 @@ impl WorldData {
             garbage_collector.pixel_updated();
         }));
         pixel_read_pass.set_sync_callback(f!(garbage_collector.pixel_synced()));
-        // TODO: We may want to enable it on weak hardware.
-        // pixel_read_pass.set_threshold(1);
+        *self.pixel_read_pass_threshold.borrow_mut() = pixel_read_pass.get_threshold().downgrade();
         let pipeline = render::Pipeline::new()
             .add(SymbolsRenderPass::new(&self.default_scene.layers))
             .add(ScreenRenderPass::new())
@@ -583,6 +585,18 @@ impl WorldData {
     pub fn collect_garbage<T: 'static>(&self, object: T) {
         self.garbage_collector.collect(object);
     }
+
+    /// Set the maximum frequency at which the pointer location will be checked, in terms of number
+    /// of frames per check.
+    pub fn set_pixel_read_period(&self, period: usize) {
+        if let Some(setter) = self.pixel_read_pass_threshold.borrow().upgrade() {
+            // Convert from minimum number of frames per pixel-read pass to
+            // minimum number of frames between each frame that does a pixel-read pass.
+            let threshold = period.saturating_sub(1);
+            info!("Setting pixel read pass threshold to {threshold}.");
+            setter.set(threshold);
+        }
+    }
 }
 
 mod js_bindings {

lib/rust/ensogl/pack/js/src/runner/config.json

@@ -58,6 +58,11 @@
           "value": false,
           "description": "Enables SpectorJS. This is a temporary flag to test Spector. It will be removed after all Spector integration issues are resolved. See: https://github.com/BabylonJS/Spector.js/issues/252.",
           "primary": false
+        },
+        "pixelReadPeriod": {
+          "value": "1",
+          "description": "Minimum number of frames from one pixel read pass to the next.",
+          "primary": false
         }
       }
     }