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

Remove obsolete FastList<T> #153

Open
VaclavElias opened this issue Aug 25, 2024 · 9 comments
Open

Remove obsolete FastList<T> #153

VaclavElias opened this issue Aug 25, 2024 · 9 comments
Assignees
Labels
engineering Makes the pull request to appear in the "Engineering" section of the next release note refactoring Code base improvements

Comments

@VaclavElias
Copy link
Collaborator

May I assign it to you?

warning: D:\Projects\GitHub\stride-community-toolkit\src\Stride.CommunityToolkit\Rendering\DebugShapes\ImmediateDebugRenderObject.cs(19,23): warning CS0618: 'FastList<ImmediateDebugRenderFeature.Renderable>' is obsolete: '.NET Lists can be faster in the latest .NET versions.'
warning: D:\Projects\GitHub\stride-community-toolkit\src\Stride.CommunityToolkit\Rendering\DebugShapes\ImmediateDebugRenderObject.cs(20,23): warning CS0618: 'FastList<ImmediateDebugRenderFeature.Renderable>' is obsolete: '.NET Lists can be faster in the latest .NET versions.'
warning: D:\Projects\GitHub\stride-community-toolkit\src\Stride.CommunityToolkit\Rendering\DebugShapes\ImmediateDebugRenderFeature.cs(295,22): warning CS0618: 'FastList<ImmediateDebugRenderFeature.InstanceData>' is obsolete: '.NET Lists can be faster in the latest .NET versions.'
warning: D:\Projects\GitHub\stride-community-toolkit\src\Stride.CommunityToolkit\Rendering\DebugShapes\ImmediateDebugRenderFeature.cs(298,22): warning CS0618: 'FastList<Matrix>' is obsolete: '.NET Lists can be faster in the latest .NET versions.'
warning: D:\Projects\GitHub\stride-community-toolkit\src\Stride.CommunityToolkit\Rendering\DebugShapes\ImmediateDebugRenderFeature.cs(299,22): warning CS0618: 'FastList<Color>' is obsolete: '.NET Lists can be faster in the latest .NET versions.'
warning: D:\Projects\GitHub\stride-community-toolkit\src\Stride.CommunityToolkit\Rendering\DebugShapes\ImmediateDebugRenderFeature.cs(302,22): warning CS0618: 'FastList<ImmediateDebugRenderFeature.LineVertex>' is obsolete: '.NET Lists can be faster in the latest .NET versions.'
warning: D:\Projects\GitHub\stride-community-toolkit\src\Stride.CommunityToolkit\Rendering\DebugShapes\ImmediateDebugRenderSystem.cs(447,54): warning CS0618: 'FastList<ImmediateDebugRenderSystem.DebugRenderable>' is obsolete: '.NET Lists can be faster in the latest .NET versions.'
warning: D:\Projects\GitHub\stride-community-toolkit\src\Stride.CommunityToolkit\Rendering\DebugShapes\ImmediateDebugRenderSystem.cs(211,22): warning CS0618: 'FastList<ImmediateDebugRenderSystem.DebugRenderable>' is obsolete: '.NET Lists can be faster in the latest .NET versions.'
warning: D:\Projects\GitHub\stride-community-toolkit\src\Stride.CommunityToolkit\Rendering\DebugShapes\ImmediateDebugRenderSystem.cs(212,22): warning CS0618: 'FastList<ImmediateDebugRenderSystem.DebugRenderable>' is obsolete: '.NET Lists can be faster in the latest .NET versions.'
@VaclavElias VaclavElias added engineering Makes the pull request to appear in the "Engineering" section of the next release note refactoring Code base improvements labels Aug 25, 2024
@Doprez
Copy link
Collaborator

Doprez commented Aug 25, 2024

Yep, I can take a look at some point within the next month.

It should be as easy as changing the types to List<T> instead and trying to use Span when iterating through the list as long as it runs on the main thread.

@Doprez
Copy link
Collaborator

Doprez commented Aug 26, 2024

So apparently not as easy as I thought. There is something Im missing in regards to memory allocations with my currebt changes but Ill take another look when my head is right.

@VaclavElias
Copy link
Collaborator Author

Thank you. Do you mean that changing types to List<T> is easy, but utilising also Span<> makes it more challenging?

@Doprez
Copy link
Collaborator

Doprez commented Aug 26, 2024

No, both should be fairly straight forward. I think my issue has to do with some of the Array extensions in use.

I switched the FastLists to regular Lists and Arrays where applicable but now the allocations are out of control. I just did some quick and dirty changes without much thought so I may just need to take a step back and retry while paying closer attention to what I am doing.

@Doprez
Copy link
Collaborator

Doprez commented Aug 26, 2024

lol yep, I figured it was the resize:
image

So I just need to work around this because the Fastlist.Resize is apparently different than the default Array.Resize

@VaclavElias
Copy link
Collaborator Author

Nice, at least, this should help to refactor FastList in other places in Stride itself as well 🙂

@Doprez
Copy link
Collaborator

Doprez commented Aug 26, 2024

Honestly, the more I look at this the more I think I was too hasty in the obsoletion PR for Stride. I could make this all work but the FastList API solves all of my current issues OOB.. maybe instead I will look into making a PR in Stride to allow to get the Items within a FastList as a Span and iterate over that instead.

@VaclavElias
Copy link
Collaborator Author

Well, I think as you said the List<> performance might be good, and is going to be still improved. We should definitely aim to replace FastList<> but if FastList<> has some extra features and benefits, we should try to bring them to List<>? Extensions or other helpers? But that just might take much more time to figure out than we anticipated? 🤣 In any case, if you will be doing anything in Stride, maybe leave there a comment which parts are missing in a regular List<> for the future explorers 🙂

@Doprez
Copy link
Collaborator

Doprez commented Aug 26, 2024

The problem came down to how nice it was to be able to have the underlying array within FastList compared to List. Unless there is an easy way to get that underlying array from the built in List some of the functionality becomes obtuse to recreate.

The best example is how silly it feels to get the Span from a List. You cant just simply do List.AsSpan like you could with an array. You need to use the CollectionsMarshal.AsSpan<T>(List<T>) which is just out of the way for anyone who wants to use it. with FastList you can just use FastList.Items.AsSpan or with an extension/change to the code it could even be Fastlist.AsSpan.
example:

        public Span<T> AsSpan()
        {
            return new Span<T>(Items);
        }

Also looking at some of the methods within FastList they may benefit from simply changing some of the iterative queries to use Span where applicable so that it is done for you.

example:

        public int FindLastIndex(int startIndex, int count, Predicate<T> match)
        {
            var num = startIndex - count;
            for (var i = startIndex; i > num; i--)
            {
                if (match(Items[i]))
                {
                    return i;
                }
            }
            return -1;
        }

to

        public int FindLastIndex(int startIndex, int count, Predicate<T> match)
        {
            var itemsSpan = AsSpan();
            var num = startIndex - count;
            for (var i = startIndex; i > num; i--)
            {
                if (match(itemsSpan[i]))
                {
                    return i;
                }
            }
            return -1;
        }

I need to do some benchmarking but in my previous tests when I first deprecated the class it was pretty simple.

I started a chat within the Discord collaborators chat as well to try and get more opinions on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engineering Makes the pull request to appear in the "Engineering" section of the next release note refactoring Code base improvements
Projects
None yet
Development

No branches or pull requests

2 participants