-
-
Notifications
You must be signed in to change notification settings - Fork 1k
refactor : Discard FastList Completely #2798
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
base: master
Are you sure you want to change the base?
refactor : Discard FastList Completely #2798
Conversation
If this change is approved, I will replace all the fastlists as much as possible :) |
Small note, the idea would be to replace all codepaths referencing the internal array directly with the CollectionsMarshal.AsSpan(List) to avoid performance degradation. 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.
public class AnimationCurveEvaluatorDirectFloatGroup : AnimationCurveEvaluatorDirectBlittableGroupBase<float> | ||
{ | ||
protected unsafe override void ProcessChannel(ref Channel channel, CompressedTimeSpan newTime, IntPtr location) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, I Adjusted the location with unsafe and unsafe
sources/engine/Stride.Assets.Models/ImportModelCommand.Animation.cs
Outdated
Show resolved
Hide resolved
sources/engine/Stride.Engine/Animations/AnimationCurveEvaluatorDirectFloatGroup.cs
Outdated
Show resolved
Hide resolved
sources/engine/Stride.Engine/Animations/AnimationCurveEvaluatorDirectFloatGroup.cs
Outdated
Show resolved
Hide resolved
sources/engine/Stride.Engine/Animations/AnimationCurveEvaluatorDirectQuaternionGroup.cs
Outdated
Show resolved
Hide resolved
sources/engine/Stride.Engine/Animations/AnimationCurveEvaluatorDirectVector3Group.cs
Outdated
Show resolved
Hide resolved
sources/engine/Stride.Engine/Animations/AnimationCurveEvaluatorDirectVector4Group.cs
Outdated
Show resolved
Hide resolved
@@ -345,11 +345,6 @@ public static void Sort<T>(ConcurrentCollector<T> collection, IComparer<T> compa | |||
Sort(collection.Items, 0, collection.Count, comparer); | |||
} | |||
|
|||
public static void Sort<T>(FastList<T> collection, IComparer<T> comparer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
currentRenderTargetsNonMSAA.Clear(); | ||
currentRenderTargetsNonMSAA.Capacity = currentRenderTargets.Count; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think:
.Resize =.Clear() + set Capacity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List<T>.EnsureCapacity
might be better.
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 :) 👍 |
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
Checklist