Skip to content
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

Speedup TryGet #232

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
22 changes: 1 addition & 21 deletions src/Arch.Benchmarks/Benchmark.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ public class Benchmark
{
private static void Main(string[] args)
{
/*
// NOTE: Can this be replaced with ManualConfig.CreateEmpty()?
#pragma warning disable HAA0101 // Array allocation for params parameter
var config = new ManualConfig()
Expand All @@ -18,28 +17,9 @@ private static void Main(string[] args)
.AddLogger(ConsoleLogger.Default)
.AddColumnProvider(DefaultColumnProviders.Instance);
#pragma warning restore HAA0101 // Array allocation for params parameter
*/



var world = World.Create();
for (var index = 0; index <= 100; index++)
{
world.Create<int>();
}

var desc = new QueryDescription().WithAll<int>();
for (var index = 0; index <= 100000; index++)
{
world.Query(in desc, (ref int i) =>
{
});
}



// NOTE: Is `-- --job` a typo?
// Use: dotnet run -c Release --framework net7.0 -- --job short --filter *IterationBenchmark*
//BenchmarkSwitcher.FromAssembly(typeof(Benchmark).Assembly).Run(args, config);
BenchmarkSwitcher.FromAssembly(typeof(Benchmark).Assembly).Run(args, config);
}
}
70 changes: 70 additions & 0 deletions src/Arch.Benchmarks/TryGetBenchmark.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
using Arch.Core;
using Arch.Core.Utils;

namespace Arch.Benchmarks;

[HtmlExporter]
[MemoryDiagnoser]
[HardwareCounters(HardwareCounter.CacheMisses)]
public class TryGetBenchmark
{
private static World _world;
private static List<Entity> _entities;

private Consumer _consumer = new();

[GlobalSetup]
public void Setup()
{
_world = World.Create();

_entities = new List<Entity>(1_000_000);
for (var index = 0; index < 1_000_000; index++)
{
_entities.Add(_world.Create(new Transform(), new Velocity()));
}
}

[Benchmark]
public void TryGetGenericRef()
{
for (var index = 0; index < _entities.Count; index++)
{
var entity = _entities[index];
var xform = _world.TryGetRef<Transform>(entity, out var exists);

if (exists)
{
_consumer.Consume(xform);
}
}
}

[Benchmark]
public void TryGetGeneric()
{
for (var index = 0; index < _entities.Count; index++)
{
var entity = _entities[index];

if (_world.TryGet<Transform>(entity, out var xform))
{
_consumer.Consume(xform);
}
}
}

[Benchmark]
public void TryGet()
{
for (var index = 0; index < _entities.Count; index++)
{
var entity = _entities[index];

if (_world.TryGet(entity, Component.GetComponentType(typeof(Transform)), out var xform))
{
_consumer.Consume(xform);
}
}
}
}
24 changes: 24 additions & 0 deletions src/Arch.Tests/WorldTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -604,6 +604,18 @@ public void Add()
That(_world.GetArchetype(entity2), Is.EqualTo(_world.GetArchetype(entity)));
That(arch, Is.EqualTo(_world.GetArchetype(entity)));
}

/// <summary>
/// Checks if generic TryGet works on entities.
/// </summary>
[Test]
public void TryGet()
{
var entity = _world.Create(new Transform());

That(_world.TryGet(entity, out Transform xform), Is.EqualTo(true));
That(_world.TryGet(entity, out Rotation rot), Is.EqualTo(false));
}
}


Expand Down Expand Up @@ -658,6 +670,18 @@ public void Add_NonGeneric()
That(_world.GetArchetype(entity2), Is.EqualTo(_world.GetArchetype(entity)));
That(arch, Is.EqualTo(_world.GetArchetype(entity)));
}

/// <summary>
/// Checks if generic TryGet works on entities.
/// </summary>
[Test]
public void TryGet_NonGeneric()
{
var entity = _world.Create(new Transform());

That(_world.TryGet(entity, Component<Transform>.ComponentType, out var xform), Is.EqualTo(true));
That(_world.TryGet(entity, Component<Rotation>.ComponentType, out var rot), Is.EqualTo(false));
}
}

/// <summary>
Expand Down
22 changes: 22 additions & 0 deletions src/Arch/Core/Archetype.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System.Buffers;
using System.Diagnostics.Contracts;
using Arch.Core.Extensions;
using Arch.Core.Extensions.Internal;
using Arch.Core.Utils;
Expand Down Expand Up @@ -299,6 +300,27 @@ internal Archetype(Signature signature)
_removeEdges = new SparseJaggedArray<Archetype>(BucketSize);
}

/// <summary>
/// Try get the index of a component within this archetype. Returns false if the archetype does not have this
/// component.
/// </summary>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
[Pure]
internal bool TryIndex<T>(out int i)
{
var id = Component<T>.ComponentType.Id;
Debug.Assert(id != -1, $"Index is out of bounds, component {typeof(T)} with id {id} does not exist in this chunk.");

Choose a reason for hiding this comment

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

The assert text should probably be updated and I don't think its actually correct, I'm not sure if -1 is even possible? I think I initially just pilfered part of the asset from Chunk.Index<T>() and never really updated the text.


if (id >= _componentIdToArrayIndex.Length)
{
i = -1;
return false;
}

i = _componentIdToArrayIndex.DangerousGetReferenceAt(id);
return i != -1;
}

/// <summary>
/// The component types that the <see cref="Arch.Core.Entity"/>'s stored here have.
/// </summary>
Expand Down
23 changes: 11 additions & 12 deletions src/Arch/Core/World.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using Arch.Core.Extensions.Internal;
using Arch.Core.Utils;
using Collections.Pooled;
using CommunityToolkit.HighPerformance;
using Schedulers;
using Component = Arch.Core.Utils.Component;

Expand Down Expand Up @@ -983,18 +984,18 @@ public ref T Get<T>(Entity entity)
[Pure]
public bool TryGet<T>(Entity entity, out T? component)
{
component = default;

var entitySlot = EntityInfo.GetEntitySlot(entity.Id);
var slot = entitySlot.Slot;
var archetype = entitySlot.Archetype;
ref var slot = ref EntityInfo.EntitySlots[entity.Id];

if (!archetype.Has<T>())
if (!slot.Archetype.TryIndex<T>(out int compIndex))
{
component = default;
return false;
}

component = archetype.Get<T>(ref slot);
ref var chunk = ref slot.Archetype.GetChunk(slot.Slot.ChunkIndex);
Debug.Assert(compIndex != -1 && compIndex < chunk.Components.Length, $"Index is out of bounds, component {typeof(T)} with id {compIndex} does not exist in this chunk.");
Copy link

@ElectroJr ElectroJr Sep 5, 2024

Choose a reason for hiding this comment

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

The assert message was also pilfered from elsewhere, and the "does not exist in this chunk" should probably be updated as it is not in Chunk method anymore

var array = Unsafe.As<T[]>(chunk.Components.DangerousGetReferenceAt(compIndex));
component = array[slot.Slot.Index];
return true;
}

Expand All @@ -1009,16 +1010,14 @@ public bool TryGet<T>(Entity entity, out T? component)
[Pure]
public ref T TryGetRef<T>(Entity entity, out bool exists)
{
var entitySlot = EntityInfo.GetEntitySlot(entity.Id);
var slot = entitySlot.Slot;
var archetype = entitySlot.Archetype;
Comment on lines -1012 to -1014

Choose a reason for hiding this comment

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

IIRC these changes were from an older arch version and aren't really required anymore. I.e., previously it was getting the entity slot twice using

        var slot = EntityInfo.GetSlot(entity.Id);
        var archetype = EntityInfo.GetArchetype(entity.Id);

but the current version doesn't seem to do that anymore.

ref var slot = ref EntityInfo.EntitySlots[entity.Id];

if (!(exists = archetype.Has<T>()))
if (!(exists = slot.Archetype.Has<T>()))
{
return ref Unsafe.NullRef<T>();
}

return ref archetype.Get<T>(ref slot);
Comment on lines -1016 to -1021

Choose a reason for hiding this comment

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

This can probably also be changed to use TryGetIndex instead of Has<T> & Get<T>. IIRC I just didn't bother update it for the SS14 benchmarks because they weren't using this method

return ref slot.Archetype.Get<T>(ref slot.Slot);
}

/// <summary>
Expand Down
Loading