Closed SaculRennorb closed 1 week ago
Thanks, this is indeed quite complicated ^^ I've been rather thinking of a solution in the middle where the SortStable method on ListExt would ClearAll, that way we would only clear when the whole sorting operation is over and not on every release. I'll run some tests and go for the one with the best perf/maintenance convenience ratio.
I've been rather thinking of a solution in the middle where the SortStable method on ListExt would ClearAll, that way we would only clear when the whole sorting operation is over and not on every release.
Well this is exactly what this pr does, but ClearAll is not something that exists on the default ArrayPool - hence the added implementation
You could also just drop the memory monitoring ive added, which would also get rid of some of the complexity, and just clear all on every sort.
Just keep in mind that clearing all arrays is still an expensive operation, and before you run it every time you really want to profile how that affects performance.
That is what I'm testing yes, removing the monitor and call ClearAll after a stable sort has been done (instead of doing a clear at every dispose).
Not sure if this is the correct target branch, but you will figure out where this should go or even if you just want to implement another alternative to this fix which i will explain at the end.
Ok, so this is a bit of a doozie:
When originally adding the new stable sort implementation I failed to realize that while cycle-efficient the Pooled arrays used by the sorting would keep objects alive that are otherwise no longer used:
The sorting uses swap space that comes form array pools. This is great in theory, but unfortunately one of the largest arrays we are sorting is of type AbstractBuffEvent. That is a reference type, which means those individual event objects are kept alive by the now unused swap buffer which we returned to the pool, because that swap buffer does not get destroyed.
In theory there already is a fix for that:
The return method on array pools has a bool parameter telling them to clear the array the get back befor storing it again, which removes those "dead" references and allows the objects to be freed - but there is an issue with this:
PooledArraySize<GW2EIEvtcParser.ParsedData.AbstractBuffEvent>: 9 Min, 33.87 Avg, 29000 Max (136971 Samples)
We only sort a few arrays, but the sorting itself uses the array pools internally. And from the metrics above you can see that it does so 136971 times! We would like to avoid clearing the array 136971 times, because for most of those operations we will be overwriting the contained references right away either way.So this is my solution for the dilemma:
I implemented a custom array pool that gets used when the input to sort is a reference type:
It implements a clear function, and the SortStable extension function (the one use-case for the stable sort implementation) checks the current memory pressure and clears the arrays, releasing references, above a certain threshold.
Now - several things im not quite happy with:
For all of these reasons it might be more desirable to just replace 99% of this with a field in PoolReturner that tells it to clear the array on returning it, and set it in the sort impl in the same way i now set a different ArrayPool.