Skip to content

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

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

Conversation

Arc-huangjingtong
Copy link
Contributor

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 Arc-huangjingtong changed the title Discard FastList Completely WIP : refactor : Discard FastList Completely May 25, 2025
@Arc-huangjingtong
Copy link
Contributor Author

If this change is approved, I will replace all the fastlists as much as possible :)

@Eideren
Copy link
Collaborator

Eideren commented May 25, 2025

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
Copy link
Contributor Author

Arc-huangjingtong commented May 25, 2025

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

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

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

@@ -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)
Copy link
Contributor Author

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

Comment on lines 475 to 477
currentRenderTargetsNonMSAA.Clear();
currentRenderTargetsNonMSAA.Capacity = currentRenderTargets.Count;

Copy link
Contributor Author

@Arc-huangjingtong Arc-huangjingtong May 25, 2025

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

Copy link
Member

@Kryptos-FR Kryptos-FR May 25, 2025

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

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

Copy link
Member

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.

@Arc-huangjingtong Arc-huangjingtong marked this pull request as ready for review June 21, 2025 15:15
@Arc-huangjingtong Arc-huangjingtong changed the title WIP : refactor : Discard FastList Completely refactor : Discard FastList Completely Jun 21, 2025
@Arc-huangjingtong
Copy link
Contributor Author

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

@Eideren
Copy link
Collaborator

Eideren commented Jun 22, 2025

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
Copy link
Contributor Author

Arc-huangjingtong commented Jun 23, 2025

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 :) 👍

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.

3 participants