refactor: Discard internal use of FastList by Arc-huangjingtong · Pull Request #2798 · stride3d/stride

Arc-huangjingtong · 2025-05-25T11:05:03Z

PR Details

[FastList] is already obsolete ,but still has some code in project ,In the long run, someone has to do this

Related Issue

#2332

Types of changes

Docs change / refactoring / dependency upgrade
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My change requires a change to the documentation.
I have added tests to cover my changes.
All new and existing tests passed.
I have built and run the editor to try this change out.

Arc-huangjingtong · 2025-05-25T11:06:50Z

If this change is approved, I will replace all the fastlists as much as possible ：）

Eideren · 2025-05-25T11:12:22Z

Small note, the idea would be to replace all codepaths referencing the internal array directly with the CollectionsMarshal.AsSpan(List) to avoid performance degradation.
So, instead of

for (int i = 0; i < keyFrames.Count; ++i)
{
   if (parentNodeIndex == 0)
       keyFrames.Items[i].Value -= PivotPosition;
   keyFrames.Items[i].Value *= ScaleImport;
}

You would do

var span = CollectionsMarshal.AsSpan(keyFrames);
for (int i = 0; i < span.Count; ++i)
{
   if (parentNodeIndex == 0)
       span[i].Value -= PivotPosition;
   span[i].Value *= ScaleImport;
}

Arc-huangjingtong · 2025-05-25T11:18:43Z

Small note, the idea would be to replace all codepaths referencing the internal array directly with the CollectionsMarshal.AsSpan(List) to avoid performance degradation.CollectionsMarshal.AsSpan(List)So, instead of
for (int i = 0; i < keyFrames.Count; ++i)
{
   if (parentNodeIndex == 0)
       keyFrames.Items[i].Value -= PivotPosition;
   keyFrames.Items[i].Value *= ScaleImport;
}
You would do
var span = CollectionsMarshal.AsSpan(keyFrames);
for (int i = 0; i < span.Count; ++i)
{
   if (parentNodeIndex == 0)
       span[i].Value -= PivotPosition;
   span[i].Value *= ScaleImport;
}

thanks ， i see ，i will change

This reverts commit 7d72ba4.

Arc-huangjingtong · 2025-05-25T13:23:52Z

+
    public class AnimationCurveEvaluatorDirectFloatGroup : AnimationCurveEvaluatorDirectBlittableGroupBase<float>
    {
-        protected unsafe override void ProcessChannel(ref Channel channel, CompressedTimeSpan newTime, IntPtr location)


BTW, I Adjusted the location with unsafe and unsafe

Arc-huangjingtong · 2025-05-25T15:45:02Z

        Sort(collection.Items, 0, collection.Count, comparer);
    }

-    public static void Sort<T>(FastList<T> collection, IComparer<T> comparer)


I delete this, because the method is 0 used , and if deleted , the scripts will easier to maintain , if not it will cause misunderstandings to others

Dispatcher is public, users could be calling this method in their project, so we should leave it for for now and remove it once we remove FastList<T>

Arc-huangjingtong · 2025-05-25T15:48:35Z

+            currentRenderTargetsNonMSAA.Clear();
+            currentRenderTargetsNonMSAA.Capacity = currentRenderTargets.Count;
+


I think:
.Resize =.Clear() + set Capacity

Unfortunately, this case isn't equivalent. In FastList<T> if the new capacity was smaller than the previous one, only its size was modified (i.e. no allocations). In List<T> in all cases (except when it's already the correct size) a new array is allocated and a copy occurs.

This will have impact on performance and was one of the reason FastList<T> was created.

Compare

stride/sources/core/Stride.Core/Collections/FastList.cs

Lines 220 to 232 in b2c29fd

public void Resize(int newSize, bool fastClear)

{

if (size < newSize)

{

EnsureCapacity(newSize);

}

else if (!fastClear && size > newSize)

{

Array.Clear(Items, newSize, size - newSize);

}

size = newSize;

}

with
https://github.com/dotnet/runtime/blob/2765f1856d7ebcc0ebd9708e7a2322f565a1fc77/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/List.cs#L97-L124

List<T>.EnsureCapacity might be better.

Arc-huangjingtong · 2025-06-21T15:22:45Z

Now ， all the [FastList] is discard but the [FastListStruct] not deleted,I think its need deleted too, maybe other pr ?

Eideren · 2025-06-22T16:52:38Z

FastListStruct isn't marked as obsolete, I haven't looked at whether it makes sense to replace that one with something better/safer so best leave it off for now and have a conversation in a new Issue about it if you want to look into that.

I should have time to review this next week end if Kryptos doesn't do it by then

Arc-huangjingtong · 2025-06-23T07:44:18Z

FastListStruct isn't marked as obsolete, I haven't looked at whether it makes sense to replace that one with something better/safer so best leave it off for now and have a conversation in a new Issue about it if you want to look into that.FastListStruct

I should have time to review this next week end if Kryptos doesn't do it by then

Yes , thanks, I maybe try it , the key question is use more modern and better ways to improve performance, rather than just delete code :) 👍

Arc-huangjingtong · 2025-07-10T16:56:29Z

👀

Eideren

Sorry about the wait, pretty good work overall, just a few things to fix and some improvements you could tackle.

Eideren · 2025-07-11T12:39:43Z

                                    var keyFrames = ((AnimationCurve<Vector3>)curve).KeyFrames;
+                                    var keyFramesSpan = CollectionsMarshal.AsSpan(keyFrames);
                                    for (int i = 0; i < keyFrames.Count; ++i)
                                    {
                                        if (parentNodeIndex == 0)
-                                            keyFrames.Items[i].Value -= PivotPosition;
-                                        keyFrames.Items[i].Value *= ScaleImport;
+                                            keyFramesSpan[i].Value -= PivotPosition;
+                                        keyFramesSpan[i].Value *= ScaleImport;
                                    }


keyFrames.Count -> keyFramesSpan.Length, using the span's length instead of the list's will allow dotnet to elide the bounds check, see https://sharplab.io/#v2:C4LghgzgtgPgAgJgIwFgBQcAMACOSAsA3OlrkgHQBKArgHbACWUApuQJL3MBOA9gA4BlbgDcGAY2YRiaEgGZcCbAGF0Ab3TZNueQ3rYAsgAoAMgwjAAPADMANjzDAAfNhsAabLfvAPASg1b1NC1g7GEwLmwIPjBabABeZR4bG2YxRh5aCH1wiAALMBtyAEEIAWjaQxsfaX8Qjx4Iw11vBnjsTEJsVosXciUeOmBOgGphhj8guuxAqZCGK2xDKJiAbQYAXXiEqwnZ4Jm9urgAdi7pQ80AX1qQ68m6m+CT7ABaJHOtO5u4HT19BBMZksngczjcHjsDl8NwOdTCEWWsQS/WSqXSmWyXDyBWKpXKlWq6EeWisDUWzS6bQ6lJ6iPIxmYtAA5sBciMxrsprC9vNFoi1ps4ttOYduRdcKcGB9DndZrKpsTNM83tLsHdLkA=

Eideren · 2025-07-11T13:18:52Z

-            currentRenderTargetsNonMSAA.Resize(currentRenderTargets.Count, false);
+            currentRenderTargetsNonMSAA.Clear();
+            currentRenderTargetsNonMSAA.EnsureCapacity(currentRenderTargets.Count);
+


This isn't quite the same, right now, the way you have it setup is that you're clearing the whole list and then making sure the list can contain at least currentRenderTargets.Count long.
Resize changes the list's capacity to hold the given amount, but also the list's length, so if it is 5 items long and you pass it 3, it will be 3 items long from then on, but it's capacity will still be 5 or whichever capacity the list had at the time.
And passing false to Resize only affects the list when decreasing its length, so when the list is 5 items in length and is changed to 3 items in length, the 4th and 5th item will be set to null/default.

Eideren · 2025-07-11T13:19:25Z

-            currentRenderTargets.Resize(renderTargets.Count, false);
+            currentRenderTargets.Clear();
+            currentRenderTargets.EnsureCapacity(renderTargets.Count);


Same as the other comment

Eideren · 2025-07-11T13:22:51Z

+            var resourceBindingsSpan = CollectionsMarshal.AsSpan(reflection.ResourceBindings);
+
            // prepare resource bindings used internally
            for (int i = 0; i < reflection.ResourceBindings.Count; i++)


See my other comment about about using the span's length directly

Eideren · 2025-07-11T13:23:32Z

-                renderTargets.Resize(validatedTargetCount, false);
-
+                renderTargets.Clear();
+                renderTargets.EnsureCapacity(renderTargets.Count);


See my other comment

Eideren · 2025-07-11T13:45:41Z

                    if (parameterKeyInfos[i].Key == layoutParameterKeyInfo.Key)
                    {
                        processedParameters[i] = true;
-                        newParameterKeyInfos.Items[i] = layoutParameterKeyInfo;
+                        newParameterKeyInfosSpan[i] = layoutParameterKeyInfo;


parameterKeyInfos.Count -> newParameterKeyInfosSpan.Length

Eideren · 2025-07-11T13:47:05Z

+            var parameterKeyInfosSpan = CollectionsMarshal.AsSpan(parameterKeyInfos);
            // Find existing first
            for (int i = 0; i < parameterKeyInfos.Count; ++i)
            {
-                if (parameterKeyInfos.Items[i].Key == parameterKey)
+                if (parameterKeyInfosSpan[i].Key == parameterKey)
                {
-                    return parameterKeyInfos.Items[i].GetObjectAccessor();
+                    return parameterKeyInfosSpan[i].GetObjectAccessor();


Same as other span length stuff

Eideren · 2025-07-11T13:50:48Z


-        private unsafe bool IsTetrahedronAllocated(int index)
+        private unsafe bool IsTetrahedronAllocated(int index, Span<Tetrahedron> tetrahedronSpan)
        {
-            fixed (Tetrahedron* tetrahedron = &tetrahedralization.Items[index])
+            fixed (Tetrahedron* tetrahedron = &tetrahedronSpan[index])
            {
                return tetrahedron->Vertices[0] != -1;
            }
        }


Entire method body can be replaced by return tetrahedronSpan[index].Vertices[0] != -1;

Eideren · 2025-07-11T13:52:21Z

            // Mark it as "unused"
-            fixed (Tetrahedron* tetrahedron = &tetrahedralization.Items[index])
+            fixed (Tetrahedron* tetrahedron = &tetrahedronSpan[index])
            {
                tetrahedron->Vertices[0] = -1;



tetrahedronSpan[index].Vertices[0] = -1;

Eideren · 2025-07-11T13:54:39Z

        {
+            var tetrahedralizationSpan = CollectionsMarshal.AsSpan(tetrahedralization);
            // Check connectivity
-            fixed (Tetrahedron* tetrahedra = tetrahedralization.Items)
+            fixed (Tetrahedron* tetrahedra = tetrahedralizationSpan)
            {
                for (int index = 0; index < tetrahedralization.Count; index++)
                {
-                    if (!IsTetrahedronAllocated(index))
+                    if (!IsTetrahedronAllocated(index, tetrahedralizationSpan))


Yep, same as the rest

Replaces inefficient Clear() calls and Count-based loops with span-based operations using CollectionsMarshal.AsSpan for better performance. Updates affected code in ForwardRenderer, RenderOutputValidator, ImportModelCommand.Animation, and Effect to use spans for clearing and iterating over lists, and adds an obsolete overload in Dispatcher to guide usage towards array-based sorting.

Refactored BowyerWatsonTetrahedralization to eliminate unsafe code and pointer arithmetic, replacing them with direct access to Span and ref variables. This improves code safety, readability, and maintainability while preserving the original logic and performance.

Replaced Count-based loops with span Length in ParameterCollection for improved performance and correctness. Removed unnecessary Clear() call on SortedRenderNodes in RenderSystem as the collection is properly cleared in Reset().

Eideren · 2025-08-28T15:33:42Z

Tweaked it a bit before merging, thanks for your work @Arc-huangjingtong

…obes

)

replace AnimationCurve

7d72ba4

Arc-huangjingtong changed the title ~~Discard FastList Completely~~ WIP : refactor : Discard FastList Completely May 25, 2025

Arc-huangjingtong added 2 commits May 25, 2025 21:02

Revert "replace AnimationCurve"

4d6c760

This reverts commit 7d72ba4.

replace AnimationCurve with the way @Eideren recommend

3d7a7c2

Arc-huangjingtong commented May 25, 2025

View reviewed changes

Kryptos-FR reviewed May 25, 2025

View reviewed changes

Arc-huangjingtong added 4 commits May 25, 2025 23:30

FastList has been completely removed

0f3e699

Update AnimationCurveEvaluatorDirectFloatGroup.cs

708d854

Update LightShaderGroupDynamic.cs

f6712c3

Update Effect.cs

b2c29fd

Arc-huangjingtong commented May 25, 2025

View reviewed changes

Kryptos-FR reviewed May 25, 2025

View reviewed changes

Comment thread sources/engine/Stride.Engine/Animations/AnimationBlender.cs

VaclavElias mentioned this pull request Jun 4, 2025

Remove obsolete FastList<T> stride3d/stride-community-toolkit#153

Closed

Arc-huangjingtong marked this pull request as ready for review June 21, 2025 15:15

Arc-huangjingtong changed the title ~~WIP : refactor : Discard FastList Completely~~ refactor : Discard FastList Completely Jun 21, 2025

replace to EnsureCapacity

abdbc3c

Eideren reviewed Jul 11, 2025

View reviewed changes

Arc-huangjingtong and others added 5 commits July 14, 2025 02:31

Fix Resize implementation

6720edc

Finish the few changes leftover in BowyerWatsonTetrahedralization.cs

1ad0b75

Fix order of operation issues, and resize leftovers

e2d810a

Copilot AI review requested due to automatic review settings August 28, 2025 12:26

This comment was marked as spam.

Sign in to view

Eideren added 3 commits August 28, 2025 14:28

Merge branch 'master' into Feat_DeprecateFastListCompletely

fc544dc

Correct OpenGL usage of FastList

11a9c12

Update tetrahedron span after adding them to the list

c91fd25

Eideren merged commit a122c71 into stride3d:master Aug 28, 2025
9 checks passed

Eideren changed the title ~~refactor : Discard FastList Completely~~ refactor: Discard internal use of FastList Aug 28, 2025

Eideren added the area-Core Issue of the engine unrelated to other defined areas label Aug 28, 2025

Eideren added a commit to Eideren/xenko that referenced this pull request Sep 14, 2025

fix: Remove usage of FastList from samples following stride3d#2798

65bbd07

xen2 pushed a commit that referenced this pull request Sep 18, 2025

fix: Remove usage of FastList from samples following #2798

a312ce2

Eideren added a commit to Eideren/xenko that referenced this pull request Oct 25, 2025

fix: Rollback regression introduced through stride3d#2798 wrt lightpr…

7453c34

…obes

Eideren mentioned this pull request Oct 25, 2025

fix: Rollback regression introduced through #2798 wrt lightprobes #2949

Merged

8 tasks

Eideren added a commit that referenced this pull request Oct 25, 2025

fix: Rollback regression introduced through #2798 wrt lightprobes (#2949

78d73dd

)

xen2 pushed a commit that referenced this pull request Dec 2, 2025

fix: Rollback regression introduced through #2798 wrt lightprobes (#2949

1e5254b

)

xen2 pushed a commit that referenced this pull request Dec 2, 2025

fix: Rollback regression introduced through #2798 wrt lightprobes (#2949

6496128

)

		currentRenderTargetsNonMSAA.Clear();
		currentRenderTargetsNonMSAA.Capacity = currentRenderTargets.Count;

	public void Resize(int newSize, bool fastClear)
	{
	if (size < newSize)
	{
	EnsureCapacity(newSize);
	}
	else if (!fastClear && size > newSize)
	{
	Array.Clear(Items, newSize, size - newSize);
	}

	size = newSize;
	}

Uh oh!

Conversation

Arc-huangjingtong commented May 25, 2025

PR Details

Related Issue

Types of changes

Checklist

Uh oh!

Arc-huangjingtong commented May 25, 2025

Uh oh!

Eideren commented May 25, 2025

Uh oh!

Arc-huangjingtong commented May 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Arc-huangjingtong May 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kryptos-FR May 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Arc-huangjingtong commented Jun 21, 2025

Uh oh!

Eideren commented Jun 22, 2025

Uh oh!

Arc-huangjingtong commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Arc-huangjingtong commented Jul 10, 2025

Uh oh!

Eideren left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

Uh oh!

Eideren commented Aug 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Arc-huangjingtong commented May 25, 2025 •

edited

Loading

Arc-huangjingtong May 25, 2025 •

edited

Loading

Kryptos-FR May 25, 2025 •

edited

Loading

Arc-huangjingtong commented Jun 23, 2025 •

edited

Loading