Added pooling for Texture2D.PlatformGetData #8384
Conversation
| @@ -251,7 +251,7 @@ private void PlatformGetData<T>(int level, int arraySlice, Rectangle rect, T[] d | |||
| // Note: for compressed format Format.GetSize() returns the size of a 4x4 block | |||
| var pixelToT = Format.GetSize() / tSizeInByte; | |||
| var tFullWidth = Math.Max(this.width >> level, 1) / 4 * pixelToT; | |||
| var temp = new T[Math.Max(this.height >> level, 1) / 4 * tFullWidth]; | |||
| var temp = GetDataPool<T>.GetArray(Math.Max(this.height >> level, 1) / 4 * tFullWidth); | |||
There was a problem hiding this comment.
So this makes the code not thread safe... but i suppose OpenGL never was?
There was a problem hiding this comment.
GetData and SetData should only execute on UI thread (line 228), so we can use this non thread safe pooling
I also thought about using ArrayPool.Shared, but it seemed that will be an overkill for this pr
| @@ -268,7 +268,7 @@ private void PlatformGetData<T>(int level, int arraySlice, Rectangle rect, T[] d | |||
| { | |||
| // we need to convert from our format size to the size of T here | |||
| var tFullWidth = Math.Max(this.width >> level, 1) * Format.GetSize() / tSizeInByte; | |||
| var temp = new T[Math.Max(this.height >> level, 1) * tFullWidth]; | |||
| var temp = GetDataPool<T>.GetArray(Math.Max(this.height >> level, 1) * tFullWidth); | |||
There was a problem hiding this comment.
Are we gonna always leave behind a stale largest texture ever allocated?
i.e. it never gets deallocated unless the size mismatches on a subsequent call, correct? This could be in MBs (size of screen).
Usually in a pool, you rent and then return. I don't see the return part here. In fact, why not just use ArrayPool like you said - it's not overkill imo?
There was a problem hiding this comment.
ArrayPool should be fine if ArrayPool.Create() is used with maxArrayLength = 4096 * 4096 (that should be enough) and maxArraysPerBucket = 1, since only one array will be used at a time
There was a problem hiding this comment.
Also maybe maxArrayLength should be int.MaxValue instead of 4096 * 4096, so array can be any length and still be saved to the pool
|
|
||
| static class GetDataPool<T> where T : struct | ||
| { | ||
| public static ArrayPool<T> pool = ArrayPool<T>.Create(int.MaxValue, 1); |
There was a problem hiding this comment.
Could this be readonly and named 'Pool` (capitalize?)...
In general looks good - will need a maintainer to review this @mrhelmut ?.
|
|
||
| static class GetDataPool<T> where T : struct | ||
| { | ||
| public static ArrayPool<T> pool = ArrayPool<T>.Create(int.MaxValue, 1); |
There was a problem hiding this comment.
Perhaps add a comment as to why int.MaxValue is being used? (I reckon the default 2^20 doesn't work well for our large texture use case?).
This should fix #8304