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

Add component existence check #216

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

Conversation

AnnulusGames
Copy link

@AnnulusGames AnnulusGames 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 :)

@AnnulusGames
Copy link
Author

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