Fix allocation on tickables #279
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/hadashia/vcontainer/CTKEgrG73cUsuiH1fJsgipwsdqmG |
| readonly EntryPointExceptionHandler exceptionHandler; | ||
| bool disposed; | ||
|
|
||
| public FixedTickableLoopItem( | ||
| IEnumerable<IFixedTickable> entries, | ||
| EntryPointExceptionHandler exceptionHandler) | ||
| { | ||
| this.entries = entries; | ||
| this.entries = entries.ToList(); |
There was a problem hiding this comment.
I think it would be better if ToList() was removed.
( It is not clearly that no copying will occur
This class is internal and has no use case other than being passed an IReadOnlyList<T>.
In this case, why not keep IReadOnlyList<T> as an argument, and use for and [] operator instead of foreach ?
( for ... statement is exactly the same as the implementation of List<>.GetEnumerator. )
There was a problem hiding this comment.
In this case, why not keep IReadOnlyList as an argument, and use for and [] operator instead of foreach ?
I like that!
| Assert.That(() => | ||
| { | ||
| tickableLoopItem.MoveNext(); | ||
| }, Is.Not.AllocatingGCMemory()); |
|
Thanks for the review! |
|
This makes sense. Thanks! |
When using Tickable, GC Allocation of 40 bytes per frame was occurring.
IEnumerable<T>.GetEnumerator()causes an implicit cast, I think.To solve this problem, I modified the LoopItem to explicitly hold the List inside.
I'd like you to see if it looks OK. 🙏