Page MenuHomeWildfire Games
Differential D4487

Atomic Fence Replacing Deprecated _ReadWriteBarrier
Needs ReviewPublic
Actions

Authored by deleualex on Feb 11 2022, 4:22 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

The _ReadWriteBarrier is an intrinsic function that is deprecated. According to MS Docs, atomic functions should be used (stating with C++11).

More details:

https://wildfiregames.com/forum/topic/70263-compiler_fence-uses-deprecated-function/

Whether we should still be using the fences or the compiler is clever enough so that they are no longer needed is under debate and subject to testing on enough platforms so that it can be properly assessed. May well be that we take them out completely and nothing happens.

Test Plan

Should pass the entire test suite, performance tests and stress tests, particularly memory usage tests (in general the nightly builds). There is no particular Unit test that can verify this change.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

deleualex requested review of this revision.Feb 11 2022, 4:22 PM
deleualex created this revision.
vladislavbelov added a subscriber: vladislavbelov.Feb 11 2022, 5:18 PM

Thanks for the patch!

Please, add context to patches: https://trac.wildfiregames.com/wiki/SubmittingPatches#Makingsomechanges . It makes reviewing easier, thanks.

particularly memory usage tests (in general the nightly builds).

We'd like to have some :)

source/lib/code_annotation.h
277

Might replace it by #include <atomic>.

278

Why not std::memory_order_acq_rel? IIRC we use it in io.h to complete both operations.

Stan added a reviewer: Restricted Owners Package.Mar 6 2022, 11:00 PM
Stan added a subscriber: Stan.

Any news?

deleualex updated this revision to Diff 19911.Mar 13 2022, 6:04 PM

According to review comment from vladislavbelov I used std::memory_order_acq_rel which is more suitable.

vladislavbelov added a comment.Apr 20 2022, 10:44 PM

Sorry for the long delay.

The patch still needs context: https://trac.wildfiregames.com/wiki/SubmittingPatches#Makingsomechanges

I've checked assembler generated by MSVC for multiple architectures. It seems std::atomic_thread_fence(std::memory_order_acq_rel) is same or more strict than _ReadWriteBarrier().

source/lib/code_annotation.h
275

That include might be replaced by #include <atomic> to avoid duplication.

phosit added a subscriber: phosit.Dec 31 2022, 5:45 PM

IMO we should remove COMPILER_FENCE and use std::atomics or std::atomic_thread_fences instead

Stan added a comment.Dec 31 2022, 5:56 PM

Feel free to update the patch :)

Revision Contents
Changeset List

PathSize
binaries/
data/
mods/
public/
gui/
credits/
texts/
programming.json
programming.json (revision 26638)
1 line
source/
lib/
code_annotation.h
code_annotation.h (revision 26638)
3 lines
debug.cpp
debug.cpp (revision 26638)
1 line
module_init.cpp
module_init.cpp (revision 26638)
3 lines
sysdep/
os/
win/
wdbg_sym.cpp
wdbg_sym.cpp (revision 26638)
3 lines
wposix/
waio.cpp
waio.cpp (revision 26638)
4 lines
wvm.cpp
wvm.cpp (revision 26638)
3 lines

Diff 19911

View Options

binaries/data/mods/public/gui/credits/texts/programming.json

Loading...
View Options

source/lib/code_annotation.h

Loading...
View Options

source/lib/debug.cpp

Loading...
View Options

source/lib/module_init.cpp

Loading...
View Options

source/lib/sysdep/os/win/wdbg_sym.cpp

Loading...
View Options

source/lib/sysdep/os/win/wposix/waio.cpp

Loading...
View Options

source/lib/sysdep/os/win/wvm.cpp

Loading...
Wildfire Games · Phabricator