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.

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

Conversation

arttu-peltonen
Copy link
Contributor

@arttu-peltonen arttu-peltonen commented Nov 23, 2021

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.

- 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
@arttu-peltonen arttu-peltonen self-assigned this Nov 23, 2021
@github-actions
Copy link

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.
Link to Yamato: https://unity-ci.cds.internal.unity3d.com/project/902/
Search for your PR branch using the search bar at the top, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

SRP Core
You could run ABV on your branch before merging your PR, but it will start A LOT of jobs. Please be responsible about it and run it only when you feel the PR is ready:
/jobDefinition/.yamato%252F_abv.yml%2523all_project_ci_trunk
Be aware that any modifications to the Core package impacts everyone in the Graphics repo so please discuss the PR with your lead.

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.

@github-actions github-actions bot added the SRP label Nov 23, 2021
@arttu-peltonen arttu-peltonen marked this pull request as ready for review November 23, 2021 12:06
Copy link
Contributor

@alex-vazquez alex-vazquez 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, 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)
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.

@@ -114,6 +119,8 @@ internal void DiscardOldSamples(int 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.

@arttu-peltonen arttu-peltonen merged commit db1e4f1 into master Nov 26, 2021
@arttu-peltonen arttu-peltonen deleted the rpw/fix-debugframetiming-gc-allocs branch November 26, 2021 12:25
sebastienlagarde pushed a commit that referenced this pull request Nov 27, 2021
* 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.
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.

4 participants