Skip to content

Add component existence check #216

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nuskey8
Copy link

@nuskey8 nuskey8 commented May 8, 2024

Add checks and exceptions for Add/Remove/Get/Set related to #174. This PR will change it to throw InvalidOperationException if you specify a component type that does not exist.

It is recommended to use exceptions instead of assertions here. These are public APIs of the library, and an exception should be thrown if the user specifies an incorrect parameter. When these defects occur during release builds, Arch makes heavy use of unsafe memory operations, which can cause crashes or unexpected behavior, making it difficult to identify the cause. For the above reasons, I changed Debug.Assert of World.Move to an exception.

The reason why I use a dedicated ThrowHelper to throw exceptions is due to optimization. This method is widely used in .NET.

@genaray
Copy link
Owner

genaray commented May 10, 2024

Noted, I think it's time to finally introduce safety checks. Im gonna look at this once I have some free time and merge it :)

@genaray
Copy link
Owner

genaray commented May 15, 2024

Just took a deeper look at it. Are there any performance drawbacks? An small benchmark would be great :)

@nuskey8
Copy link
Author

nuskey8 commented Jul 2, 2024

Sorry for the delay with my assignment. Below is a simple benchmark code and its results.

using Arch.Core;

namespace Arch.Benchmarks;

[MemoryDiagnoser]
public class AddRemoveBenchmark
{
    World world;
    Entity entity;

    [GlobalSetup]
    public void Setup()
    {
        world = World.Create();
        entity = world.Create();
    }

    [GlobalCleanup]
    public void Cleanup()
    {
        world.Dispose();
    }

    [Benchmark]
    public void AddRemove()
    {
        for (int i = 0; i < 100000; i++)
        {
            world.Add<Velocity>(entity);
            world.Remove<Velocity>(entity);
        }
    }
}

before:
スクリーンショット 2024-07-02 10 08 19

after:
スクリーンショット 2024-07-02 10 07 12

Surprisingly, the modified version is slightly faster. However, this difference is negligible and, in practice, there is almost no difference in execution speed.

@genaray
Copy link
Owner

genaray commented Jul 12, 2024

Thanks that looks promising! Could you also benchmark your version against the arch nugget? It might make the benchmark more complicated, but the goal is to benchmark both versions in the same benchmark running once. Not before and after ^^ This would verify it and we can merge it :)

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