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

Conversation

metalgearsloth
Copy link
Contributor

@metalgearsloth metalgearsloth commented Sep 3, 2024

Code done by @ElectroJr not me, I just PRd it.

Without the GetChunk change:
image
With GetChunk change:
image

Left is master, right is after changes (the non-generic TryGet was just my system being weird, after multiple runs it was equivalent).

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.

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

Comment on lines -1012 to -1014
var entitySlot = EntityInfo.GetEntitySlot(entity.Id);
var slot = entitySlot.Slot;
var archetype = entitySlot.Archetype;

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.

Comment on lines -1016 to -1021
if (!(exists = archetype.Has<T>()))
if (!(exists = slot.Archetype.Has<T>()))
{
return ref Unsafe.NullRef<T>();
}

return ref archetype.Get<T>(ref slot);

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants