Preventing resize overuse in SortVisibleSpans #1646
Conversation
There was a problem hiding this comment.
Good catch! hsTArray::SetCount() would only reallocate if more elements were requested. It never deallocated. Cyan's code was more like using std::vector::reserve() instead of resize(), therefore. Like reserve(), SetCount() also does not initialize the newly created elements. Unfortunately, I'm uncertain if assigning to an index that hasn't been initialized is defined behavior. It works fine on release builds in MSVC, but it might assert in debug mode. So, this might be the best fix. Maybe @zrax can chime in here?
|
I tried resize as an alternative when working on this - and it caused index out of range failures on macOS. I'd also be curious if there was a better way to do this. This is also not the only code that was refactored to use resize. There are other uses even in this function. The use here was by far the highest contributors in the performance graph. I can do an analysis of the remaining uses. |
Just to make sure - do you mean that you tried |
|
Yes, sorry, I tried reserve and it produced out of bounds errors. |
Adding an if check to resize the buffers only when there is a risk of an overflow. Every pass through is resizing the scratch buffers in SortVisibleSpans. Resize is completely deallocating and reallocating the buffers. This is resulting in a ~5% performance drag - so this change results in a ~5% performance improvement on the main loop. It’s possible this is a result of 78c0b48 which moved this storage over to std. I don’t know if the Cyan array implementation would have handled this. I don’t see any reason this would create issues - there doesn’t seem to be code that relies on the size of the vectors. The vectors only need to be correctly sized to fit all the data required.
colincornaby
force-pushed
the
Prevent-Sort-Visible-Spans-Reallocations
branch
from
fc2aad9 to
14a636e
Compare
|
I've rebased this branch. Upon further review of the performance data - I think this actually produces closer to a 10% speed up on the main render loop. So this should be a pretty good improvement. |
Adding an if check to resize the buffers only when there is a risk of an overflow.
Every pass through is resizing the scratch buffers in SortVisibleSpans. Resize is completely deallocating and reallocating the buffers. This is resulting in a ~5% performance drag - so this change results in a ~5% performance improvement on the main loop.
It’s possible this is a result of 78c0b48 which moved this storage over to std. I don’t know if the Cyan array implementation would have handled this.
I don’t see any reason this would create issues - there doesn’t seem to be code that relies on the size of the vectors. The vectors only need to be correctly sized to fit all the data required.