Skip to content

Ensure DebugFrameTiming doesn't allocate #6368

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 26, 2021
Merged

Ensure DebugFrameTiming doesn't allocate #6368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4d6ff0e
Ensure DebugFrameTiming doesn't allocate
arttu-peltonen Nov 23, 2021
78ca8c2
Add assert to avoid infinite loop on invalid usage.
arttu-peltonen Nov 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions com.unity.render-pipelines.core/Runtime/Debugging/DebugFrameTiming.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
//#define RTPROFILER_DEBUG

using System;
using System.Collections.Generic;
using System.Linq;
using UnityEngine;

//#define RTPROFILER_DEBUG

namespace UnityEngine.Rendering
{
public class DebugFrameTiming
Expand All @@ -13,8 +13,8 @@ public class DebugFrameTiming
const string k_MsFormatString = "{0:F2}ms";
const float k_RefreshRate = 1f / 5f;

internal FrameTimeSampleHistory m_FrameHistory = new();
internal BottleneckHistory m_BottleneckHistory = new();
internal FrameTimeSampleHistory m_FrameHistory;
internal BottleneckHistory m_BottleneckHistory;

/// <summary>
/// Size of the Bottleneck History Window in number of samples.
Expand All @@ -29,6 +29,12 @@ public class DebugFrameTiming
FrameTiming[] m_Timing = new FrameTiming[1];
FrameTimeSample m_Sample = new FrameTimeSample();

public DebugFrameTiming()
{
m_FrameHistory = new FrameTimeSampleHistory(sampleHistorySize);
m_BottleneckHistory = new BottleneckHistory(bottleneckHistorySize);
}

/// <summary>
/// Update timing data from profiling counters.
/// </summary>
Expand Down Expand Up @@ -158,16 +164,16 @@ public void RegisterDebugUI(List<DebugUI.Widget> list)
new DebugUI.IntField
{
displayName = "Frame Time Sample History Size",
getter = () => SampleHistorySize,
setter = (value) => { SampleHistorySize = value; },
getter = () => sampleHistorySize,
setter = (value) => { sampleHistorySize = value; },
min = () => 1,
max = () => 100
},
new DebugUI.IntField
{
displayName = "Bottleneck History Size",
getter = () => BottleneckHistorySize,
setter = (value) => { BottleneckHistorySize = value; },
getter = () => bottleneckHistorySize,
setter = (value) => { bottleneckHistorySize = value; },
min = () => 1,
max = () => 100
},
Expand Down
9 changes: 9 additions & 0 deletions com.unity.render-pipelines.core/Runtime/Debugging/FrameTiming/FrameTimeBottleneck.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,23 @@ internal struct BottleneckHistogram
/// </summary>
internal class BottleneckHistory
{
public BottleneckHistory(int initialCapacity)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to me that BottleneckHistory and FrameTimeSampleHistory are pretty the same. I would encourage you to do a base generic class.

internal class History<T>
{
 List<T> m_Samples = new();
 public History(int initialCapacity):
 public void DiscardOldSamples(int sampleHistorySize);
 public void Add(T sample);
 public void Clear();
}

class BottleneckHistory : History<PerformanceBottleneck> and FrameTimeSampleHistory : History<FrameTimeSample>

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, I had the same thought when making the same changes to both files, but decided to not tackle the issue right now :) I do agree this would make sense though. I'll make a note to come back to this later.

{
m_Bottlenecks.Capacity = initialCapacity;
}

List<PerformanceBottleneck> m_Bottlenecks = new();

internal BottleneckHistogram Histogram;

internal void DiscardOldSamples(int historySize)
{
Debug.Assert(historySize > 0, "Invalid sampleHistorySize");

while (m_Bottlenecks.Count >= historySize)
m_Bottlenecks.RemoveAt(0);

m_Bottlenecks.Capacity = historySize;
}

internal void AddBottleneckFromAveragedSample(FrameTimeSample frameHistorySampleAverage)
Expand Down
9 changes: 9 additions & 0 deletions com.unity.render-pipelines.core/Runtime/Debugging/FrameTiming/FrameTimeSample.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ internal FrameTimeSample(float initValue)
/// </summary>
class FrameTimeSampleHistory
{
public FrameTimeSampleHistory(int initialCapacity)
{
m_Samples.Capacity = initialCapacity;
}

List<FrameTimeSample> m_Samples = new();

internal FrameTimeSample SampleAverage;
Expand Down Expand Up @@ -112,8 +117,12 @@ void ForEachSampleMember(ref FrameTimeSample aggregate, FrameTimeSample sample,

internal void DiscardOldSamples(int sampleHistorySize)
{
Debug.Assert(sampleHistorySize > 0, "Invalid sampleHistorySize");

while (m_Samples.Count >= sampleHistorySize)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would check the sampleHistorySize to have a correct value. As I might call it with -1 and get an infinte loop.

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 catch, I fixed this one right away.

m_Samples.RemoveAt(0);

m_Samples.Capacity = sampleHistorySize;
}

internal void Clear()
Expand Down