Skip to content

Conversation

arttu-peltonen
Copy link
Contributor

@arttu-peltonen arttu-peltonen commented Aug 24, 2021

Purpose of this PR

This PR introduces the Realtime Profiler, which allows users to see cpu/gpu frame timings and bottlenecks detected out of these timings.

profiler2


Details

Full description of the features with screenshots in this document.

This PR depends on New FrameTiming Backend PR by @bandures, which hasn't landed in trunk yet. Therefore all testing & Yamato runs are done with the latest commit from trunk/profiler/feature/universal-frame-timing-v3. EDIT: Trunk PR has landed, works at latest trunk now.


Testing status (status mid-September)

Matrix describing what I've tested:

Platform GFX API Build type Tested? Status
Windows DX11 Editor ✔️ 👍
Windows DX11 Player ✔️ 👍
Windows DX12 Editor ✔️ 👍
Windows DX12 Player ✔️ 👍
Windows Vulkan Editor ✔️ 👍
Windows Vulkan Player ✔️ 👍
Mac Metal Editor ✔️ Render thread & GPU timing missing
Mac Metal Player ✔️ GPU timing missing
Mac GL Editor ✔️ GPU timing missing
Mac GL Player ✔️ GPU timing missing, intermittent invalid main thread times
Android Vulkan Player ✔️ 👍
Android GLES3 Player ✔️ GPU timing missing (Google Pixel 2)
iOS Player
PS4 Player ✔️ Render thread timing missing
PS5 Player
X1 Player
XSX Player
Switch Player
VR/XR Editor
VR/XR Player

Tested functionality on HDRP and URP, in editor, editor playmode and player builds.

- FrameTimingManager looks like the likely candidate where the frametime numbers would eventually come from. It required 3 things to start working:
-- Switched gfx backend to D3D12 that has an implementation for FrameTimingManager (unlike D3D11)
-- Enabled FrameTimingStats from project settings to see numbers.
-- Made standalone build work, where we also get GPU numbers.
- Upgraded project to use latest trunk version of Unity to make native debugging easier.
- Moved all code under com.unity.render-pipelines.core & renamed stuff
- Removed all assets & prefabs from SRP_RealTimeProfiler project (gameobjects still manually placed in scene)
- Test project now uses local packages instead of official ones. Temporarily removed com.unity.ui due to incompatibility with 2021.1
- Runtime components don't need com.unity.ui package anymore.
- All GOs are now created automatically through script, so profiling should work in any project.
- Asset loading for builds still to be fixed.
- Removed UI Toolkit assets from project.
- Added Theme & PanelSettings assets to Resources under the package, load using Resources.Load<>() to remove UnityEditor namespace use.
…nterValue API.

- For this purpose, RT Core needs to temporarily depend on com.unity.profiling.core.
- Update test project to trunk version.
- Contains some hacks (workaround for Gfx tests leaking gameobjects, disabled in Player builds due to lack of "is FrameTiming enabled" API)
… Foldouts.

- Rearrange new Frame Stats in a column layout, show Avg/Min/Max for each value.
- Rearrange HDRP detailed stats in a column layout, with CPU/CPUInline/GPU as the columns.
- Pinning ValueTuples to PersistentCanvas is visualized by separating each column value with "/". Not ideal but needed something compact.
- Fix a bug with Foldouts that allowed runtime and editor collapsed state getting out of sync.
- Duplicate profiler recorders were being added to the array which also messed up averages.
- Fix bug where RayCountManager would never get enabled.
- Use integer instead of string keys for Dictionary. 1.75ms -> 0.5ms (with deep profiling enabled, true cost is lower).
- Also fixed bug where per-second average values were not updated unless runtime UI was active, meaning pinned values wouldn't update.
- Sometimes, backend may not provide a valid value for every counter, but provides 0 instead. These invalid values must be ignored when computing min/avg.
- The refactor also hides implementation details better and reduces duplication. No performance impact (both old & new logic never exceeded 0.01ms).
Copy link
Contributor

@jenniferd-unity jenniferd-unity left a comment

Choose a reason for hiding this comment

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

Approved! great job @arttu-peltonen !
I have added a few comments I am sure you will know how to tackle

- Fix 15KB per frame alloc caused by compiler generating a new Func<float,float,float> every frame from a local function. Made them static Func objects directly to avoid this.
- Avoid new(struct) in DebugFrameTimings.cs. Not sure why this causes an allocation, but use member variables to avoid it regardless.
- Don't access Input.touches (which allocates) on old InputManager, unless touches are present.
@arttu-peltonen
Copy link
Contributor Author

Update: Added Performance Analysis section to the PR notes doc.

/// <returns>The formatted value string.</returns>
public virtual string FormatString(object value)
{
return string.IsNullOrEmpty(formatString) ? $"{value}" : string.Format(formatString, value);

Choose a reason for hiding this comment

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

I guess this would be a good place to switch to MutableString once available...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, absolutely. I've spent some time trying to eliminate allocs but this was the one place I couldn't easily fix without removing support for format strings altogether, which I would like to keep. Great to hear that there's work for a non-allocating StringBuilder/String.Format that I could use in the future!

@fabien-unity
Copy link
Collaborator

I quickly tried with XR and the report seems accurate.
However, the debug display is only visible in the mirror view (desktop). This is something not related to this PR but we should think about improving the debug render to be compatible with XR inside the headset.

@arttu-peltonen
Copy link
Contributor Author

I quickly tried with XR and the report seems accurate.
However, the debug display is only visible in the mirror view (desktop). This is something not related to this PR but we should think about improving the debug render to be compatible with XR inside the headset.

Thanks! Yeah, I agree. I have only tested with MockHMD, not headset, and haven't investigated how XR is supported with uGUI.

# Conflicts:
#	com.unity.render-pipelines.core/Runtime/Debugging/DebugUpdater.cs
#	com.unity.render-pipelines.core/Runtime/Debugging/Prefabs/Resources/DebugUICanvas.prefab
#	com.unity.render-pipelines.high-definition/Runtime/Debug/DebugDisplay.cs
# Conflicts:
#	com.unity.render-pipelines.core/Runtime/Debugging/IDebugDisplaySettingsData.cs
#	com.unity.render-pipelines.universal/Runtime/Debug/DebugDisplaySettingsCommon.cs
@arttu-peltonen
Copy link
Contributor Author

Note: there's a new version of the trunk PR now, so please use profiler/feature/universal-frame-timing-v3 branch for testing.

Copy link
Contributor

@eh-unity eh-unity left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link

@aleksandrasdzikia aleksandrasdzikia left a comment

Choose a reason for hiding this comment

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

Looks good to me. Tested on two different macs (intel and apple silicon). Both behaved as expected. Apple silicon on Rosetta had issues, but not related to this PR. tested both Metal and GL. Different URP projects were used - Universal Rendering Examples, URP Template, Viking Village. Behaviour of the frame stats were accurate to the Display stats.

Overall, very useful tool. Only one suggestion I have is from UI/UX perspective. It would make sense to not hide "Display Stats" if it's not in Play Mode. I would add some info there like: "Please, press play to display the stats" or something similar. Otherwise, it might look confusing if it's the first time of using it. Other than that, looks good!

@arttu-peltonen
Copy link
Contributor Author

arttu-peltonen commented Oct 21, 2021

Profiling backend PR has landed in trunk, running some yamato tests against trunk 71d43f7a9c283602dfa9ca30d1612a5f96669ce6 now as a sanity check.

@arttu-peltonen arttu-peltonen requested a review from a team as a code owner October 21, 2021 11:21
@arttu-peltonen
Copy link
Contributor Author

Yamato status before merge:

  • Nightly URP is green (except 035_Shader_TerrainShaders, unrelated failure)
  • PR HDRP is green (except 1550_3dsMax, fixed on master)

Copy link
Contributor

@phi-lira phi-lira left a comment

Choose a reason for hiding this comment

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

Approving on yamato state. Terrain failures are known issue on master.

Copy link
Contributor

@Verasl Verasl left a comment

Choose a reason for hiding this comment

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

LGTM

# Conflicts:
#	com.unity.render-pipelines.high-definition/CHANGELOG.md
# Conflicts:
#	com.unity.render-pipelines.universal/CHANGELOG.md
#	com.unity.render-pipelines.universal/Runtime/Debug/DebugDisplaySettingsCommon.cs
#	com.unity.render-pipelines.universal/Runtime/Debug/DebugDisplaySettingsLighting.cs
#	com.unity.render-pipelines.universal/Runtime/Debug/DebugDisplaySettingsMaterial.cs
#	com.unity.render-pipelines.universal/Runtime/Debug/DebugDisplaySettingsRendering.cs
@arttu-peltonen arttu-peltonen merged commit 86f0904 into master Oct 22, 2021
@arttu-peltonen arttu-peltonen deleted the srp/realtimeprofiler-proto branch October 22, 2021 09:21
unity-emilk pushed a commit that referenced this pull request Oct 14, 2022
This PR fixes https://jira.unity3d.com/browse/UUM-13827.

Sometimes after a build, DebugManager can fail to register debug systems in the editor because of the way `Debug.IsDebugBuild` works. To avoid crashes because of this, we shouldn't use  `Debug.IsDebugBuild` but instead use conditional compilation. This has been fixed in 2022+ but a backport was never done.

This was originally fixed in [an earlier PR](#5419) ([commit](5e4f2dd)). This PR only takes the relevant parts of the original PR required to fix the bug because the entire PR cannot be backported.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.