reducev performance #2757
Replies: 2 comments 7 replies
-
|
I agree, that was a really expensive fix, there must be something better. I think this is where that extra seq came from: #617 ie. it was part of the revised seq mechanism. It'd be good to get a reproducer for this problem so we can test possible improvements. I've tried commenting out the extra seq and trying to provoke an out of order read, but not managed it yet. There's this issue as well from a month later: #639 that one finds and fixes a couple of small cache size issues, so (perhaps??) it makes the addition of the seq unnecessary now? I've tried using But I've not been able to provoke an out of order read. |
Beta Was this translation helpful? Give feedback.
-
|
Ah that's a shame. But good to have a reproducer, nice! |
Beta Was this translation helpful? Give feedback.
-
|
That doesn't reproduce on this PC :( I'll try a laptop. |
Beta Was this translation helpful? Give feedback.
-
|
Ah, I could not reproduce this with |
Beta Was this translation helpful? Give feedback.
-
|
Here's another benchmark: https://gist.github.com/kleisauke/8e27833e3c0ac0b4f35200203cc36bf4 (on Windows with a 5000x5000 image). It's curious to see that this performance degradation affects the |
Beta Was this translation helpful? Give feedback.
-
|
I've updated the above benchmark gist for libvips 8.13.0 and latest |
Beta Was this translation helpful? Give feedback.
-
|
There's an issue and sample code here with a possible improvement #3754 |
Beta Was this translation helpful? Give feedback.
-
|
To save you a click, the idea is that we should have some thread-private storage containing an allocation number -- a small integer assigned in order from 0 by the threadpool allocation function (this can go in the base class) and order by that. sequential should stall threads which are out of order, and only let them though by strict allocation number. This means out of order reads should be impossible, except in the cases where there's very large non-locality. |
Beta Was this translation helpful? Give feedback.
-
The extra
vips_sequentialinreducev, introduced in commit 3ca2068, could slow down multi-threaded resize operations. This may cause--vips-concurrency 1orVIPS_CONCURRENCY=1to perform better than the same operation executed with full concurrency. See for example this gist:https://gist.github.com/kleisauke/7d6f194cde52737ce8a494ee0a5e55e7
On that gist, removing this extra
vips_sequentialmakes the C-path ofreducev~67% faster, and the AVX2/SSE4.1 paths (see thesimdbranch) varying between ~40% and ~52% faster. The multi-threaded timings are all inline after that, with at most 16ms difference.This discussion was opened to find possible alternatives for resolving the "out of order read" errors under heavy load, which were fixed with that commit.
(This was initially discussed on Gitter, but I thought it would be better to track it here)
Beta Was this translation helpful? Give feedback.
All reactions