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

Dynamic app resampling and better performance measurements. #6595

Merged
merged 15 commits into from
May 28, 2023

Conversation

wdanilo
Copy link
Member

@wdanilo wdanilo commented May 7, 2023

Pull Request Description

This PR introduces several new tools for GPU-related performance measurements and a new, low-resolution rendering mode which should increase the GUI speed on old, weak hardware.


Proper measurement of GPU performance.

WARNING! Currently (2023) this extension is only available in Chrome Desktop.

A new way to measure GPU performance have been introduced. It bases on the ExtDisjointTimerQueryWebgl2 WebGL extension, which is pretty tricky to use. First of all, it was banned from all browsers some time ago because of the ["GLitch" exploit] (https://www.vusec.net/projects/glitch). However, as Site Isolation shipped in Chrome on Desktop, the extension was re-enabled there because the only data that could be read was data that came from the same origin. To learn more, see: https://bugs.chromium.org/p/chromium/issues/detail?id=1230926.


Automatic low/high resolution modes.

The application resolution mode is automatically switched between low and hight. In the low resolution mode, the device pixel ratio will be set to 1.0 even on high-dpi screens. This will result in faster rendering (every 4-th pixel) and a more blurry image of the GUI. I believe that the blurriness of the fonts can be decreased in this mode by tweaking shaders, and it can be performed in a separate PR. The switch happens after a few frames were rendered slow/fast enough. The performance of the rendering is measured using the new performance measurements described above. In case the app is run not in Chrome, the FPS will be used instead of the proper measurements, which might cause the app to switch to low-resolution mode even if the bottleneck is CPU, not GPU.

Screen.Recording.2023-05-08.at.01.22.53.mov

Improvements to the performance monitor.

The performance monitor was extended with additional information:

  • The "frame time" was renamed to "GPU + CPU time".
  • The "CPU time" is a new measurements which reports the time spent on CPU-related operations, excluding GPU ones.
  • The "GPU time" is a new measurement which reports the time spent on GPU-related operations, excluding CPU ones.
  • Idle time is a new measurement which reports how much free time was left during rendering of the frame. Please note, that currently this measurement is broken and it can report negative values, as we need to enable high-resolution timers in Enso.

Please note, that the division between CPU and GPU time is not known immediately and we can get it with a few frames delay. That's why you can observe sometimes in the performance monitor a greyed-out value for a few frames, which turns into a real value after some time:

Screen.Recording.2023-05-08.at.01.32.26.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.

@wdanilo wdanilo marked this pull request as ready for review May 8, 2023 00:42



/// A vector with a constant max size, that if full, keeps its element in a loop.
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 understand this line. It does not keep elements, but rather drops the old ones when the capacity is full, doesn't it?

self.vec.get_mut(index)
}

/// get the last element of the vector, if any.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// get the last element of the vector, if any.
/// Get the last element of the vector, if any.

Copy link
Contributor

@kazcw kazcw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have one concern: If the user's hardware renders slower than the slow-frame threshold in high-resolution mode, but faster than the slow-frame threshold in low-resolution mode, would we switch modes every 8 frames?

/// Check the current value in order to draw it with warning or error if it exceeds the allowed
/// thresholds.
/// Check the current value in order to draw it with warning or error color if it exceeds the
/// allowed thresholds.
pub fn check(&self, stats: &StatsData) -> ValueCheck {
let value = self.value(stats);
ValueCheck::from_threshold(self.warn_threshold, self.err_threshold, value)
}

/// Minimum size of the size the sampler should occupy in the performance monitor view.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

size of the size (not new code, but let's fix it)

@@ -1167,6 +1182,18 @@ impl Scene {
}
}

/// Run the GPU profiler. If the result is [`None`], either the GPU context is not initialized
/// ot the profiler is not available at the current platform. In case the resulting vector
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// ot the profiler is not available at the current platform. In case the resulting vector
/// or the profiler is not available at the current platform. In case the resulting vector

@@ -164,7 +158,7 @@ macro_rules! emit_if_integer {
($other:ty, $($block:tt)*) => ();
}

/// Emits the StatsData struct, and extends StatsWithTimeProvider with accessors to StatsData
/// Emits the StatsData struct, and extends Stats with accessors to StatsData
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Emits the StatsData struct, and extends Stats with accessors to StatsData
/// Emits the [`StatsData`] struct, and extends [`Stats`] with accessors to [`StatsData`]

Comment on lines 53 to 55
/// The time threshold for switching to low resolution mode. It will be used on platforms which
/// allow proper GPU time measurements (currently only Chrome).
const LOW_RESOLUTION_MODE_GPU_TIME_THRESHOLD: f64 = 1000.0 / 30.0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the units of this value?

@vitvakatu
Copy link
Contributor

vitvakatu commented May 8, 2023

QA Report 🔴

Opening the performance monitor (ctrl+alt+~) with open DevTools immediately triggers unwrap:

https://github.com/enso-org/enso/pull/6595/files#diff-9d7b5a2d329a89fe5b9eb16627c503593a7db720088eb79f993d1e58bd4828b6R693

Everything else looks fine, I didn't manage to get a significant FPS drop to trigger the downscaling, though.

GitHub
Pull Request Description This PR introduces several new tools for GPU-related performance measurements and a new, low-resolution rendering mode which should increase the GUI speed on old, weak hard...

@kazcw
Copy link
Contributor

kazcw commented May 9, 2023

I just tested this on the 2020 M1 Air. No blockers, other than the same panic Ilya has encountered.

  • GPU time shows as N/A. Maybe the API isn't supported on this machine's drivers.
  • I don't think the reduced-pixel-ratio is threshold is reached on this hardware. FPS rarely goes below 30 for more than one consecutive frame, no matter what I'm doing.
  • It would be nice to have a chart in the performance panel showing pixel-ratio history. Not essential though, could be added in future.

Side notes on the Air, since this is my first performance testing on it: It's not a FPS problem. We're hitting reasonable frame times under all workloads. The application feels slow because the component browser is reacting to hover changes slowly. The faster I pass over the entries with the mouse, the farther it lags behind. This indicates that events triggered by the mouseover are queuing, and we're trying to process all of them even if they are related to outdated inputs. If we remove whatever mechanism is causing hover-initiated events to queue and only try to draw whatever is currently appropriate, that will fix the sluggishness on this machine.

@wdanilo
Copy link
Member Author

wdanilo commented May 9, 2023

@kazcw thanks for all the information. I will fix the things and reply more later. For now just a short note – as you are working on lowering draw call amount, let's finish this first. Then, having these machines, let's carefully investigate all slowdowns (probably CPU-related) and let's focus on fixing them. I'll be doing that starting later today – I'll probably start with the issue with component browser.

@vitvakatu vitvakatu mentioned this pull request May 10, 2023
2 tasks
@farmaazon farmaazon linked an issue May 10, 2023 that may be closed by this pull request
2 tasks
@sylwiabr
Copy link
Member

@wdanilo what is the status here?

@wdanilo
Copy link
Member Author

wdanilo commented May 12, 2023

@sylwiabr ready for re-testing!

@vitvakatu
Copy link
Contributor

QA Report 🟢

Looks good to me!

I'm unsure if it is correct, but I only see CPU + Idle and GPU timings if the dev-tools console is opened. Otherwise, they show N/A.

@wdanilo wdanilo mentioned this pull request May 15, 2023
2 tasks
@sylwiabr
Copy link
Member

@wdanilo can we merge it?

@wdanilo
Copy link
Member Author

wdanilo commented May 26, 2023

Yes, sorry Sylwia, I stared doing another task and this slipped somehow. I need to refresh it, I will do it before Monday.

@wdanilo wdanilo merged commit 6b7cf8e into develop May 28, 2023
22 of 23 checks passed
@wdanilo wdanilo deleted the wip/wd/dynamic-app-resampling branch May 28, 2023 23:42
Procrat added a commit that referenced this pull request May 30, 2023
…le-6756-6804

* develop: (22 commits)
  Coalesce graph editor view invalidations (#6786)
  Append warnings extracted before tail call execution (#6849)
  Detect and override hooks of the same kind (#6842)
  Dynamic app resampling and better performance measurements. (#6595)
  Show spinner when opening/creating a project, take #2 (#6827)
  Infrastructure for testing inter project imports and exports (#6840)
  Only initialise visualisation chooser if it is used. (#6758)
  Allow casting a Mixed column into a concrete type (#6777)
  Stop graph editing when in full-screen visualization mode (#6844)
  Handle `show-dashboard` event (#6837)
  Fix some dashboard issues (#6668)
  Fix JWT leak (#6815)
  Fix "set username" screen (#6824)
  Fallback to opened date when ordering projects (#6814)
  Various test improvements to increase coverage and speed things up (#6820)
  do not activate nested dropdowns together (#6830)
  Clearly select single specialization with enum dispatch pattern (#6819)
  Prevent incorrect application of list widget on incompatible expressions (#6771)
  Update GraalVM to 22.3.1 JDK17 (#6750)
  Import/export syntax error have more specific messages (#6808)
  ...
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.

Debug logs left in the console
4 participants