-
Notifications
You must be signed in to change notification settings - Fork 847
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.
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
Conversation
- Setting capacity in the constructor prevents allocations at subsequent frames (can fail unit tests when Rendering Debugger window is open) - Fix some debug code that is behind a disabled ifdef
Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed. SRP Core Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure. |
There was a problem hiding this 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, if you want you can apply mi comments in another PR, and get the GC issue fixed as soon as possible.
@@ -33,6 +33,11 @@ internal struct BottleneckHistogram | |||
/// </summary> | |||
internal class BottleneckHistory | |||
{ | |||
public BottleneckHistory(int initialCapacity) |
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
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.
@@ -114,6 +119,8 @@ internal void DiscardOldSamples(int sampleHistorySize) | |||
{ | |||
while (m_Samples.Count >= sampleHistorySize) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
* Ensure DebugFrameTiming doesn't allocate - Setting capacity in the constructor prevents allocations at subsequent frames (can fail unit tests when Rendering Debugger window is open) - Fix some debug code that is behind a disabled ifdef * Add assert to avoid infinite loop on invalid usage.
Purpose of this PR
Case 1382817. Fix memory allocations by DebugFrameTiming. This is can fail gfx unit tests if you happen to have the Rendering Debugger window open. With this PR, that should no longer happen.
Testing status
Tested locally according to repro steps in the ticket: In HDRP_Tests project, 1101_Unlit fails to GCAlloc issues before fix if Rendering Debugger is open, and passes afterwards.
NOTE! Seems like 1101_Unlit_Smooth_Distortion still fails to a different issue if you have Rendering Debugger open (though not 100% - sometimes only happens after you run multiple tests). This seems to be somehow related to FrameSettingsHistory and is a separate issue.