Minor Performance Regression Fixes #314
Conversation
MACHINE_FENCE macro. Somehow this results in better performance for some chapel benchmarks.
insertinterestingnamehere
force-pushed
the
perf
branch
from
26f1469 to
59b2236
Compare
| @@ -87,7 +87,7 @@ using std::memory_order_relaxed; | |||
|
|
|||
| #include "macros.h" | |||
|
|
|||
| #define MACHINE_FENCE atomic_thread_fence(memory_order_acq_rel); | |||
There was a problem hiding this comment.
Is change is needed for correctness or for performance?
There was a problem hiding this comment.
Surprisingly, performance. I'm still confused as to how/why this is happening, but for the chapel benchmark I'm using to track the regression there's a modest but consistent performance improvement from using the sequentially consistent fences instead of just acquire release consistent. This matches what the old assembly based version was doing prior to b122234 anyway, but it's still weird.
There was a problem hiding this comment.
I'm not aware of anything in our codebase that ought to actually require sequential consistency for correctness' sake. The fact that sequential consistency is beneficial here seems to indicate that there's something suboptimal going on internally. This patch at least corrects the downstream performance regression though.
insertinterestingnamehere
merged commit af98f00
into
sandialabs:release-1.22-pre
Backporting two more performance fixes from sandialabs/qthreads#314. [Contributed by @insertinterestingnamehere. Reviewed and merged by @jabraham17]
These are some minor performance fixes to help fix regressions downstream
In particular, this removes an unnecessary thread fence in the IO code, and updates the MACHINE_FENCE macro to use a stronger type of memory fence (since this somehow results in better performance in practice).
The trylock patch is just a simplification that I wrote while attempting to debug the performance regressions. That should be performance neutral.