Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
37 changes: 33 additions & 4 deletions Explorer/Assets/DCL/Landscape/Systems/RenderGroundSystem.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ public sealed partial class RenderGroundSystem : BaseUnityLoopSystem
private readonly MaterialPropertyBlock materialProperties;
private readonly GrassIndirectRenderer? grassIndirectRenderer;

private (NativeArray<int> instanceCounts, NativeList<Matrix4x4> transforms)? nativeContainers;

private static readonly int PARCEL_SIZE_ID = Shader.PropertyToID("_ParcelSize");
private static readonly int MIN_DIST_OCCUPANCY_ID = Shader.PropertyToID("_MinDistOccupancy");
private static readonly int OCCUPANCY_MAP_ID = Shader.PropertyToID("_OccupancyMap");
Expand Down Expand Up @@ -99,11 +101,31 @@ private void RenderGroundInternal(Camera camera)
return;
}

NativeArray<int> instanceCounts = new NativeArray<int>(
landscapeData.GroundMeshes.Length, Allocator.TempJob);
// Allocated once and never resized: instanceCounts holds one slot per ground mesh kind,
// and GroundMeshes is enum-indexed (GroundMeshPiece) with a private setter, so its length
// is fixed for the lifetime of the app. transforms is a NativeList and grows on demand.
NativeArray<int> instanceCounts;
NativeList<Matrix4x4> transforms;

if (nativeContainers == null)
{
instanceCounts = new NativeArray<int>(
landscapeData.GroundMeshes.Length, Allocator.Persistent);

transforms = new NativeList<Matrix4x4>(
landscapeData.GroundInstanceCapacity, Allocator.Persistent);

nativeContainers = (instanceCounts, transforms);
}
else
{
(instanceCounts, transforms) = nativeContainers.Value;

NativeList<Matrix4x4> transforms = new NativeList<Matrix4x4>(
landscapeData.GroundInstanceCapacity, Allocator.TempJob);
for (int i = 0; i < instanceCounts.Length; i++)
instanceCounts[i] = 0;

transforms.Clear();
}

var generateGroundJob = new GenerateGroundJob
{
Expand Down Expand Up @@ -160,7 +182,14 @@ private void RenderGroundInternal(Camera camera)

startInstance += instanceCount;
}
}

protected override void OnDispose()
{
if (nativeContainers == null)
return;

(NativeArray<int> instanceCounts, NativeList<Matrix4x4> transforms) = nativeContainers.Value;
instanceCounts.Dispose();
transforms.Dispose();
}
Comment on lines 193 to 195

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P2] Null nativeContainers after disposal for double-dispose safety. After Dispose(), the field still holds a reference to the disposed containers. Setting it to null makes the null-check on line 189 a reliable guard against double-dispose and use-after-free if OnDispose is ever called twice or if RenderGroundInternal runs after disposal due to a lifecycle ordering bug.

Suggested change
instanceCounts.Dispose();
transforms.Dispose();
}
instanceCounts.Dispose();
transforms.Dispose();
nativeContainers = null;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"reference": "GUID:da80994a355e49d5b84f91c0a84a721f"
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,240 @@
using System;
using System.Linq;
using System.Reflection;
using DCL.Landscape.Systems;
using NUnit.Framework;
using Unity.Collections;
using Unity.PerformanceTesting;
using UnityEngine;

namespace DCL.Landscape.Tests.PerformanceTests
{
/// <summary>
/// <see cref="RenderGroundSystem.RenderGroundInternal"/> holds its native ground-instance
/// containers — <c>NativeArray&lt;int&gt; instanceCounts</c> and
/// <c>NativeList&lt;Matrix4x4&gt; transforms</c> — as reused <see cref="Allocator.Persistent"/>
/// fields allocated once and cleared each frame (array zero-cleared, list <c>Clear()</c>ed) rather
/// than fresh <see cref="Allocator.TempJob"/> containers allocated and disposed every frame the
/// ground is visible; both are released in an <c>OnDispose</c> override.
///
/// <para>
/// Following the repo's isolation-benchmark convention (see the sibling
/// <c>GrassScatterConstantUploadPerformanceTest</c> and
/// <c>NearbyAudioPositionHotPathPerformanceTest</c>): the production system needs a live camera,
/// ITerrain, Cinemachine preset and Arch world to run, none of which exist headless. So the metric
/// this test targets — per-frame native-container allocation churn — is measured on the exact
/// container lifecycle, covered from three angles:
/// (1) a structural reflection check that the reused fields and <c>OnDispose</c> exist on
/// <see cref="RenderGroundSystem"/>;
/// (2) a behavioral check that reproduces <c>GenerateGroundJob</c>'s partial InstanceCounts write
/// (GenerateGroundJob.cs:63-83) to prove the per-frame clear is load-bearing — without it, reusing
/// the array ghosts stale counts from the previous frame;
/// (3) a deterministic allocation-count invariant plus <c>Measure.Method</c> SampleGroups exposing
/// the interop/allocator cost the reuse avoids.
/// </para>
/// </summary>
[Category("Performance")]
public class RenderGroundContainerReusePerformanceTest
{
private const int MESH_COUNT = 3;

private const int GROUND_INSTANCE_CAPACITY = 65536;


[Test]
public void Fix_PromotesContainersToReusedPersistentFields_WithTeardown()
{
Type sys = typeof(RenderGroundSystem);
const BindingFlags FLAGS = BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.DeclaredOnly;

FieldInfo[] fields = sys.GetFields(FLAGS);

Assert.That(fields.Any(f => f.FieldType == typeof(NativeArray<int>)), Is.True,
"RenderGroundSystem must hold instanceCounts as a reused NativeArray<int> field " +
"rather than a per-frame Allocator.TempJob local.");

Assert.That(fields.Any(f => f.FieldType == typeof(NativeList<Matrix4x4>)), Is.True,
"RenderGroundSystem must hold transforms as a reused NativeList<Matrix4x4> field " +
"rather than a per-frame Allocator.TempJob local.");
Comment on lines +52 to +58

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] Reflection assertions will always fail. The field nativeContainers is typed (NativeArray<int>, NativeList<Matrix4x4>)? — i.e., Nullable<ValueTuple<…>>. No field in RenderGroundSystem has FieldType == typeof(NativeArray<int>) because the tuple wrapper changes the reflected type. Both assertions evaluate to false → both fail → this test provides no regression protection.

Suggested change
Assert.That(fields.Any(f => f.FieldType == typeof(NativeArray<int>)), Is.True,
"RenderGroundSystem must hold instanceCounts as a reused NativeArray<int> field " +
"rather than a per-frame Allocator.TempJob local.");
Assert.That(fields.Any(f => f.FieldType == typeof(NativeList<Matrix4x4>)), Is.True,
"RenderGroundSystem must hold transforms as a reused NativeList<Matrix4x4> field " +
"rather than a per-frame Allocator.TempJob local.");
Assert.That(
fields.Any(f => f.FieldType == typeof((NativeArray<int>, NativeList<Matrix4x4>)?)),
Is.True,
"RenderGroundSystem must hold instanceCounts and transforms as a reused " +
"nullable tuple field (NativeArray<int>, NativeList<Matrix4x4>)? " +
"rather than per-frame Allocator.TempJob locals.");


MethodInfo? onDispose = sys.GetMethod("OnDispose", FLAGS);
Assert.That(onDispose, Is.Not.Null,
"RenderGroundSystem must override OnDispose to release the persistent containers, " +
"else they leak native memory on every realm transition.");
}


private static void PopulateLikeJob(NativeArray<int> instanceCounts, NativeList<Matrix4x4> transforms,
int[] sortedMeshIndices)
{
var instanceCount = 0;
var meshIndex = 0;

foreach (int mi in sortedMeshIndices)
{
if (meshIndex < mi)
{
instanceCounts[meshIndex] = instanceCount;
meshIndex = mi;
instanceCount = 0;
}

instanceCount++;
transforms.Add(Matrix4x4.identity);
}

instanceCounts[meshIndex] = instanceCount;
}

[Test]
public void ReusedInstanceCounts_WithoutClear_GhostsStaleSlots_ClearFixesIt()
{
var counts = new NativeArray<int>(MESH_COUNT, Allocator.Persistent);
var transforms = new NativeList<Matrix4x4>(GROUND_INSTANCE_CAPACITY, Allocator.Persistent);

try
{
PopulateLikeJob(counts, transforms, new[] { 0, 0, 1, 1, 2 });
Assert.That(counts.ToArray(), Is.EqualTo(new[] { 2, 2, 1 }), "frame 1 baseline counts");

transforms.Clear();
PopulateLikeJob(counts, transforms, new[] { 0, 0, 0 });

Assert.That(counts[1] != 0 || counts[2] != 0, Is.True,
"Reusing InstanceCounts without a per-frame clear leaves stale non-zero counts " +
"in slots 1/2 from the previous frame.");

for (var i = 0; i < counts.Length; i++) counts[i] = 0;
transforms.Clear();
PopulateLikeJob(counts, transforms, new[] { 0, 0, 0 });

Assert.That(counts.ToArray(), Is.EqualTo(new[] { 3, 0, 0 }),
"After the per-frame clear the reused array matches a freshly-zeroed TempJob allocation " +
"(no ghost instances).");
}
finally
{
counts.Dispose();
transforms.Dispose();
}
}


[Test]
public void PersistentReuseWithClear_MatchesPerFrameFreshAllocation()
{
int[][] frames =
{
new[] { 0, 0, 1, 2 },
new[] { 0 },
Array.Empty<int>(),
new[] { 1, 1, 1, 2, 2 },
new[] { 0, 0, 0, 1, 2 },
};

var reuseCounts = new NativeArray<int>(MESH_COUNT, Allocator.Persistent);
var reuseTransforms = new NativeList<Matrix4x4>(GROUND_INSTANCE_CAPACITY, Allocator.Persistent);

try
{
for (var f = 0; f < frames.Length; f++)
{
var freshCounts = new NativeArray<int>(MESH_COUNT, Allocator.TempJob);
var freshTransforms = new NativeList<Matrix4x4>(GROUND_INSTANCE_CAPACITY, Allocator.TempJob);

if (f > 0)
{
for (var i = 0; i < reuseCounts.Length; i++) reuseCounts[i] = 0;
reuseTransforms.Clear();
}

PopulateLikeJob(freshCounts, freshTransforms, frames[f]);
PopulateLikeJob(reuseCounts, reuseTransforms, frames[f]);

Assert.That(reuseCounts.ToArray(), Is.EqualTo(freshCounts.ToArray()),
$"InstanceCounts diverged from a fresh allocation on frame {f}");
Assert.That(reuseTransforms.AsArray().ToArray(), Is.EqualTo(freshTransforms.AsArray().ToArray()),
$"transforms diverged from a fresh allocation on frame {f}");

freshCounts.Dispose();
freshTransforms.Dispose();
}
}
finally
{
reuseCounts.Dispose();
reuseTransforms.Dispose();
}
}


[Test, Performance]
public void ContainerLifecycle_PersistentReuse_IsAllocationFreePerFrame()
{
const int FRAMES = 300;

Measure
.Method(() =>
{
var counts = new NativeArray<int>(MESH_COUNT, Allocator.TempJob);
var transforms = new NativeList<Matrix4x4>(GROUND_INSTANCE_CAPACITY, Allocator.TempJob);

for (var i = 0; i < MESH_COUNT; i++) counts[i] = i;

counts.Dispose();
transforms.Dispose();
})
.SampleGroup("PerFrame_TempJob_alloc_dispose")
.WarmupCount(5).MeasurementCount(100).Run();

var reuseCounts = new NativeArray<int>(MESH_COUNT, Allocator.Persistent);
var reuseTransforms = new NativeList<Matrix4x4>(GROUND_INSTANCE_CAPACITY, Allocator.Persistent);

Measure
.Method(() =>
{
for (var i = 0; i < reuseCounts.Length; i++) reuseCounts[i] = 0;
reuseTransforms.Clear();
for (var i = 0; i < MESH_COUNT; i++) reuseCounts[i] = i;
})
.SampleGroup("PerFrame_Persistent_reuse")
.WarmupCount(5).MeasurementCount(100).Run();

reuseCounts.Dispose();
reuseTransforms.Dispose();

var tempJobAllocations = 0;

for (var f = 0; f < FRAMES; f++)
{
var c = new NativeArray<int>(MESH_COUNT, Allocator.TempJob);
tempJobAllocations++;
var t = new NativeList<Matrix4x4>(GROUND_INSTANCE_CAPACITY, Allocator.TempJob);
tempJobAllocations++;
c.Dispose();
t.Dispose();
}

var reuseAllocations = 0;
var pc = new NativeArray<int>(MESH_COUNT, Allocator.Persistent);
reuseAllocations++;
var pt = new NativeList<Matrix4x4>(GROUND_INSTANCE_CAPACITY, Allocator.Persistent);
reuseAllocations++;

for (var f = 0; f < FRAMES; f++)
{
for (var i = 0; i < pc.Length; i++) pc[i] = 0;
pt.Clear();
}

pc.Dispose();
pt.Dispose();

Assert.That(tempJobAllocations, Is.EqualTo(2 * FRAMES),
"A fresh TempJob allocation per frame allocates two native containers every frame.");
Assert.That(reuseAllocations, Is.EqualTo(2),
"The persistent containers are allocated exactly once across all frames " +
"(zero per-frame allocations on the ground path).");
}
}
}
Loading