DDraceNetwork - developer

DDraceNetwork

Development / developer

Development discussion. Logged to https://ddnet.tw/irclogs/ Connected with DDNet's IRC channel, Matrix room and GitHub repositories — IRC: #ddnet on Quakenet | Matrix: #ddnet-developer:matrix.org GitHub: https://github.com/ddnet

Between 2021-10-15 00:00:00Z and 2021-10-16 00:00:00Z

@Deleted User: the ping worked fine. It just would be nice if one runs "bam" that no errors occur. You can also simply disable client in the bam.lua or if you want to work a on a modded client fixxing this small issue and get it to build should be an easy task. But id really recommend basing clients on latest ddnet source and keep merging into it because its the best.

that what i wanted to do, i have some free time soon (edited)

07:53

merging to the latest code is a good idea and that will fix some bugs like the 15/16 players in the server list

@heinrich5991 do u know if we use volatile to define atomic variables in our code?

09:53

cuz apparently The volatile types do not provide inter-thread synchronization, memory ordering, or atomicity.

09:53

we should use _Atomic or std::atomic

09:53

Image attachment

09:54

https://en.cppreference.com/w/c/language/atomic

yes, I know that volatile for thread synchronization is wrong

09:54

I eliminated some in the job code when reworking it

09:55

> rg volatile src/ | cat
src/game/server/gamecontext.cpp:    static volatile int ReentryGuard = 0;
src/game/server/gamecontext.cpp:    static volatile int ReentryGuard = 0;
src/base/system.cpp:    *((volatile unsigned *)0) = 0x0;
src/engine/server/server.cpp:    static volatile int s_ReentryGuard = 0;
src/engine/client/client.h:    volatile int m_GfxState;
src/engine/client/sound.cpp:static volatile int m_SoundVolume = 100;
src/engine/client/backend_sdl.h:    CCommandBuffer *volatile m_pBuffer;
src/engine/client/backend_sdl.h:    volatile bool m_Shutdown;

09:55

3) is UB anyway, 1,2,4) volatile can be dropped

:o

5) m_GfxState is unused, can be removed

09:57

6) m_SoundVolume should be an atomic, its access weirdly but not completely locked

09:58

7) m_pBuffer I don't know

09:59

8) m_Shutdown just replace with an std::atomic_bool

09:59

@Ryozuki pull request when?

[ddnet/ddnet] Issue opened: #4216 Volatile cleanup

The volatile types do not provide inter-thread synchronization, memory ordering, or atomicity. we should use _Atomic or std::atomic ``` ❯ rg volatile game/server/gamecontext.cpp 3657: static volatile int ReentryGuard = 0; 3684: static volatile int ReentryGuard = 0; base/system.cpp 126: *((volatile unsigned *)0) = 0x0; engine/client/backend_sdl.h 67: CCommandBuffer *volatile m_pBuffer; 68: volatile bool m_Shutdown; engine/client/client.h 269: volatile int m_GfxState; ...

making a issue first

xd

to remember

09:59

xd

10:00

ill try looks like something ez

10:03

@heinrich5991

Image attachment

10:03

what do i do here

10:03

i think this is used to remove

10:03

a compiler warning

10:04

i think we can use abort

10:05

https://stackoverflow.com/questions/35734759/builtin-trap-when-to-use-it

10:06

abort would be a more portable way in regular of killing the process immediately due to a bug; __builtin_trap, I suspect, is there because abort is a library function and GCC can't always count on linking on the standard library.

@Deleted User src/engine/client/backend_sdl.h: CCommandBuffer *volatile m_pBuffer;

10:16

maybe u know how to fix this

[ddnet/ddnet] Pull request opened: #4217 Remove missused volatile

Fixes #4216 Still need to know how to fix CCommandBuffer *volatile m_pBuffer;

Checklist

[x] Tested the change ingame
[ ] Provided screenshots if it is a visual change
[ ] Tested in combination with possibly related configuration options
[ ] Written a unit test if it works standalone, system.c especially
[ ] Considered possible null pointers and out of bounds array indexing
[x] Changed no physics that affect existing maps
[ ] Tested the change with [ASan+UBSa...

@heinrich5991 i found about this from this post xD https://www.reddit.com/r/rust/comments/q7wys2/how_to_translate_c_volatile_to_rust/

also saw that thread

https://www.reddit.com/r/Steam/comments/q7imkz/officially_grand_theft_auto_iii_san_andreas_and/

r/Steam - Officially Grand Theft Auto: III, San Andreas and Vice Ci...

3,125 votes and 429 comments so far on Reddit

10:40

lol

10:40

i had gta san andreas

10:40

but they removed it

10:40

Officially Grand Theft Auto: III, San Andreas and Vice City were removed today by Rockstar Games to release the remaster of these games, as far as is known, only in their own launcher.

10:41

oh i still have it

10:41

xd

10:41

but u cant buy it anymore

Hm that whole CCommandBuffer thing looks rather over engineered

11:06

I think(tm) it's safe to replace the whole thing with a condition variable and an atomic-compare-exchange

11:08

Hm, we don't seem to have condition variables in the code, so weird

https://en.cppreference.com/w/cpp/atomic/atomic/compare_exchange

11:18

:o

11:18

c++20 changed some stuff from object representation to value representation

11:18

whathever that means xd

Ryozuki

:o

why are you surprised that there is a compare_exchange?

Learath2

why are you surprised that there is a compare_exchange?

i knew it existed in rust but i thought it was some modern stuff

11:19

xd

11:19

well i dont know much about atomics so idk

Ah, it's one of the fundamental atomic operations

https://doc.rust-lang.org/std/sync/atomic/struct.AtomicBool.html#method.compare_and_swap

AtomicBool in std::sync::atomic - Rust

A boolean type which can be safely shared between threads.

11:20

better naming imho xd

Ryozuki

c++20 changed some stuff from object representation to value representation

As far as I remember from the standard this is not an issue for trivial types

Image attachment

11:20

oh lol

11:20

they use same name now then

11:20

xd

iirc it only affects things like NaNs or padding bits

:o

Learath2

Hm that whole CCommandBuffer thing looks rather over engineered

so do u want to do it

Mh, not really

11:22

Maybe tonight

11:23

idk, I'm not feeling the best, don't really feel like doing anything

no problem its not a priority anyway

11:24

maybe ill take a look at it later

If I do tho, would you prefer I expose C++ condition variables through a C like interface as we do for everything else or should I just break tradition and use std directly?

im for using std

Directly? I'd be using std either way

ye directly

11:25

the c like interface seems like waste of dev time

11:25

unless some stuff is not cross-platform

11:25

and requires lot of platform specific code

I think we target > C++11 already, so dont need anything platform specific here

bluekitty

11:26

yay using stuff from 2011

11:26

greenthing

11:26

we living the modern dream

The only reason I proposed the C like interface was to keep with the style. I think I prefer directly using it aswell

c++17 is already 4 years old

11:26

If only all platforms supported 17 there is that one hack I really want to drop

which one doesnt

Mh, heinrich posted a list in an issue once. I dont remember

https://en.cppreference.com/w/cpp/compiler_support

cpp/compiler support

11:28

gcc clang msvc support it

11:28

apple clang too

Now that we have system.cpp I think we can turn some of our wrappers into better raii ones

Ryozuki

gcc clang msvc support it

The issue is that one LTS distro shipped a gcc too old iirc

11:29

Could be CentOS

well CentOS is essentially dead

11:30

since that thing hapened

11:30

iirc

11:30

CentOS Linux 8 will reach End of Life on December 31, 2021.

11:31

centos comes with gcc 8

11:31

which supports c++17

Is it just me that doesn't like this new modern "search" we have nowadays?

wdym

I search for C++17 in our issues and I honestly have no confidence in the results anymore

11:32

In the past before things like ElasticSearch you used to get a nice clean match when you search for stuff

c++17 would allow use to use the filesystem api too

11:32

xd

11:32

https://en.cppreference.com/w/cpp/filesystem

I wonder if that simple situation could be made lockless now

11:40

Hm, you could but it'd use a spinlock, which is very undesirable

To use the equivalent to what volatile is under x86, you have to use relaxed memory order https://github.com/ddnet/ddnet/blob/fdf265bc4f26bb10325936a87cc7128d0b9c166b/src/engine/client/backend/opengl/backend_opengl.cpp#L604-L605

ddnet/backend_opengl.cpp at fdf265bc4f26bb10325936a87cc7128d0b9c166...

DDraceNetwork, a cooperative racing mod of Teeworlds - ddnet/backend_opengl.cpp at fdf265bc4f26bb10325936a87cc7128d0b9c166b · ddnet/ddnet

C++20 adds a cute atomic::wait, wrapping it in a mutex and a cv all for you, wish we could use C++20

writting to m_SoundVolume is locked, and reading is fine

11:58

i doubt you need atomic or volatile

Yep, that one is safe without either. You could also try to get rid of that lock, but don't need to

maybe just use a bool smth like HasBuffer (@Ryozuki)

@Deleted User e4d67636 src/engine/client/backend_sdl.h: CCommandBuffer *volatile m_pBuffer;

12:00

instead of checking if(m_pBuffer)

12:01

but i guess you could somehow also fix it more elegant

12:01

if no buffer is there the thread has to wait anyway

There are some gfx commands that are unused for some reason

12:08

CMD_SIGNAL, CMD_RUNBUFFER

And the processor fragments don't have a pointer back to the processor processing them, this design is quite odd

never even noticed them xDwe also do a full hardware fence in the threadfunc in the backend, doesn't look like thats really a good idea (@Learath2)

CMD_SIGNAL, CMD_RUNBUFFER

Eh, the fence is probably getting emitted whether we like it or not due to the semaphores around

12:32

I will get rid of the volatile and just use a cv and a mutex. Don't really want to bother with making this lock-free

Mh, I want to re-do way too much of this, maybe someone else should do it, or we should just get rid of the volatile and call it a day

12:57

Like why is CMD_SWAP part of CMDGROUP_CORE while it's clearly handled by SDL and thus belongs in CMDGROUP_PLATFORM_SDL?

12:58

Or why do we have 2 different CMD_SHUTDOWNs?

Yeah, ok I don't want to touch this

13:22

@Deleted User e4d67636 you are more familiar with this part, I think you should do it

cmd swap should theoretically stay out of SDL, e.g. vulkan doesn use the SDL function

13:24

and actually its also a opengl function, wglSwapBuffers or glxSwapBuffers

13:27

these commands are specific commands not related to command buffer i think (@Learath2)

Or why do we have 2 different CMD_SHUTDOWNs?

13:27

well not part of the original class atleast

My initial plan was to also get rid of m_Shutdown, and replace it with a shutdown command, but that's not easy to get in there either

13:29

I think you can just nuke the volatile there and it'll be fine due to both the semaphores on either side emiting a full memory fence and us force emiting one in the middle

let me look

Is it pretty or efficient, no, will it work, I think so

IsIdle is the problem

13:31

i think i'd just not set m_pBuffer in the graphic thread

13:31

and just redesign it to only check such stuff in the main thread

Use the RunBuffer command instead to set it inside the thread?

13:31

Mh actually that won't work as it'd need to be inside a buffer

13:32

Ah I see what you mean, well how will the gfx thread signal that it's done with this buffer? A new member variable?

i dunno xd

13:34

probs still need it, but a boolean defs has an advatange over using a pointer

You could lock while checking isidle

13:36

futexes are fairly fast

13:37

in this case we also have fairly low contention, maybe just spin on an atomic CCommandBuffer *?

13:38

Though than the gfx thread could preempt the main thread, causing large lags

well atomic with relaxed memory order is pretty much volatile under x86

13:50

so that would work

maybe i'd rethink the whole work flow of the threads, e.g. have one sync point, and try to minimize all kinds of atomic/locks at all, most stuff only write changes there, so no fences in the normal executation path I generally like this pipeline design, very much like GPUs like it

14:07

swapping buffers at one point and then work with the other buffer in a read-only mode :D

Learath2

I will get rid of the volatile and just use a cv and a mutex. Don't really want to bother with making this lock-free

how does one make lock-free stuff

abuse atomics

there must be a downside right

they can garantuee memory order

otherwise everything would be lock free

it doesnt need to be faster than locking

lock-free algorithms usually turn out slower

:o

it's also more complex to design them

yeah its really use case specific

i see

14:49

A memory barrier, also known as a membar, memory fence or fence instruction, is a type of barrier instruction that causes a central processing unit (CPU) or compiler to enforce an ordering constraint on memory operations issued before and after the barrier instruction. This typically means that operations issued prior to the barrier are guaranteed to be performed before operations issued after the barrier.

14:49

is this what u mean by fences?

14:49

learning here kek

yeah its more like a memory barrier true

14:49

a fence is more like an object i think

14:50

in a pipeline for example u put a fence and can know when your implementation arrived at it

14:50

e.g. see if the fence was signaled

:o

"A memory barrier, also known as a membar, memory fence or fence instruction, is a type of barrier instruction that causes a central processing unit (CPU) or compiler to enforce an ordering constraint on memory operations issued before and after the barrier instruction."

nice u copied what i copied

oh yeah

14:52

rip

monkalaugh

14:53

why did u migrate to matrix btw

i play hide and seek discord edition

and chillerdragon forced me xd

@Ryozuki: read and understand this https://en.cppreference.com/w/cpp/atomic/memory_orderthen the rest is clear automatically

Brokenlink

did it remove some newline ?

15:04

https://en.cppreference.com/w/cpp/atomic/memory_order

cpp/atomic/memory order

memory orders are annoying to learn

16:45

I found https://gcc.gnu.org/wiki/Atomic/GCCMM/AtomicSync fairly helpful in grasping them

I wrote a blender add-on for importing Teeworlds/DDNet maps :) https://gitlab.com/Patiga/twmap-blender

poggers

1

heh, that's cool

16:50

@Ryozuki it's quite hard to reason about even the simplest things with the actual formal descriptions

16:51

Like why locks cause modifications to be visible on both sides

:o

16:51

ill save this link on my obsidian vault

While it is indeed correct that we can drop volatile here, it's only safe because the lock is there forcing the compiler to emit a fence and the locking operations synchronize. The read happens-before the write because the write is sequenced-before the unlock, which happens-before the lock (as it synchronizes-with it) that is sequenced-before the read. You don't need to assume anything about the compilers behaviour. The abstract machine prescribes correct behaviour and thus the as-if rule guarantees the optimizer can't overreach.

Here is the one I did for m_SoundVolume (edited)

:o

and fwiw while I have a vague idea why things happen the way they do, I can't possibly reason about stuff like this off the top of my head. I usually open the standard when I'm not sure if something really happens in the order I expect

nice, show some example :D (@Patiga)

I wrote a blender add-on for importing Teeworlds/DDNet maps :) https://gitlab.com/Patiga/twmap-blender

https://www.youtube.com/watch?v=88xtJ_jLVM8

17:27

https://www.youtube.com/watch?v=gVWF1N1e-xo

17:28

two animations I made with it, they are also linked in the readme

ah nice :D

Exported 202 message(s)