Open VaclavElias opened 2 weeks ago
Doprez
commented
2 weeks ago 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
commented
2 weeks ago 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
commented
2 weeks ago Thank you. Do you mean that changing types to List<T> is easy, but utilising also Span<> makes it more challenging?
Doprez
commented
2 weeks ago 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
commented
2 weeks ago lol yep, I figured it was the resize:
So I just need to work around this because the Fastlist.Resize is apparently different than the default Array.Resize
VaclavElias
commented
2 weeks ago Nice, at least, this should help to refactor FastList in other places in Stride itself as well 🙂
Doprez
commented
2 weeks ago 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
commented
2 weeks ago 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
commented
2 weeks ago 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.
May I assign it to you?