JGR wrote:

The uploaded diffs somehow ended up as 0 bytes, uploading again...

Attachments

• 1-link-graph-lag-pause.diff (12.07 KiB)
• 2-link-graph-join-multiple-jobs.diff (1.72 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/6470#comment14195

fonsinchen wrote:

I like the lag/pause patch, but there are some stylistic issues with it:

1. The IsFinished() method should be renamed as the name is quite ambiguous, now. Make it NeedsToBeJoined() or similar.
2. Line wrapping could be improved, in particular the comment in LinkGraphSchedule::Run() is hard to read.
3. The "if" cascade in StateGameLoop_LinkGraphPauseControl() can be simplified.
4. The "extern" declaration in StateGameLoop() is unnecessary. Instead, StateGameLoop_LinkGraphPauseControl() should be declared in linkgraphschedule.h, which is already included in openttd.cpp.

The multi-join patch is somewhat problematic. I did this on purpose because the merging back of link graph state takes place on the main thread and joining multiple components in a row causes more lag than only joining one. I understand that this can hold up processing of the job queue and increase the delay between game changes and link graph reactions. Maybe some more intelligent mechanism could be found that joins at most N link graph edges at a time or triggers the joining more often when the queue is long.

This comment was imported from FlySpray: https://bugs.openttd.org/task/6470#comment14217

fonsinchen wrote:

Unfortunately the flag transfer between threads is undefined behavior in C++11. Also, even before C++11, the hardware is allowed to take an arbitrarily long time to actually make the write in the link graph thread visible to the read in the main thread. The compiler might even realize that the write is never done in the main thread and optimize the whole thing away. We should use a mutex there for older compilers, and a std::atomic with a stricter memory model for C++11. That is somewhat annoying, of course. Maybe I can come up with something better.

This comment was imported from FlySpray: https://bugs.openttd.org/task/6470#comment14219

fonsinchen wrote:

It's terribly ugly, but "volatile" would probably do the trick here. As we cannot generally use C++11 atomics, we cannot establish any proper synchronization without a using a mutex. volatile does exactly the right thing on MSVC, and at least prevents the compiler from optimizing the whole thing away on other platforms. Even though not specifically guaranteed, it's probably safe to assume that the change to job_completed becomes available to the main thread fairly soon after the worker thread has terminated. The CPU's cache coherency protocol should take care of that, provided that we actually reread the value from memory (or cache) every time we need it.

The # ifdef'd atomic operations are fairly useless in their current form as the relaxed memory model doesn't actually give us the guarantees we are looking for here.

This comment was imported from FlySpray: https://bugs.openttd.org/task/6470#comment14220

JGR wrote:

On your first comment:

1. Perhaps IsScheduledToBeJoined() ?
2. Noted, I'll see about reflowing that comment, though it probably needs re-writing anyway given your second and third comments.
3. Agreed. Looking at it now, the else if clause and its nested ifs can be collapsed.
4. Agreed.

As for merging multiple link graph jobs, merging two instead of one where available could be a way forward.
It prevents the queue from being backlogged indefinitely, but shouldn't cause an excessive latency spike.

For comments 2 and 3:
On MSVC volatile reads/writes have acquire/release semantics, which is definitely correct, but stricter than necessary.
Relaxed ordering semantics are sufficient as there are no other writes by the link graph worker thread which need to be ordered with respect to the visibility in the main thread of the change to the job_completed flag, until the worker thread exits which acts as a full memory barrier. There is only one shared write, so any ordering will do for both the reader and writer.
On non MSVC platforms volatile can be assumed to be at least relaxed ordering, and to prevent the compiler removing the load/store.
An ordinary instead of atomic write should also be OK as there is no write contention, so volatile ought to be good enough.

This comment was imported from FlySpray: https://bugs.openttd.org/task/6470#comment14221

JGR wrote:

I've attached an updated version of the lag/pause patch (diff 1), with changes based on the comments above.

For the multiple join patch (diff 2), instead of joining as many jobs as possible in the same tick, the patch now adds a second join check at a later date_fract in the same day as the original join check.
As joins are now checked more often than jobs are spawned, any backlog will be gradually reduced, without joining multiple jobs in a tick or excessively increasing the rate at which jobs are joined.

Attachments

• 1-link-graph-lag-pause-v2.diff (12.30 KiB)
• 2-link-graph-join-multiple-jobs-v2.diff (2.51 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/6470#comment14239

@JGRennison is this still something you want to pursue? :) @LordAro is interested in getting it through.

Patch 1 is the interesting one really. I'm happy to make further updates as necessary.
It would be useful to know what the policy on C++11 currently is with reference to atomic operations and fonsinchen's comments above.

It's still somewhat up in the air with regards to how C++11+ is used, but now that the compile farm fully supports C++11 properly, and that -std=c++11 is forced on in the makefile anyway, I think you should be good to go. I imagine no one would complain if you used anything C++14 either.

I seem to recall some issues with using with MinGW, but I just tested that and it seems to be working fine, so go nuts :)

Closed in favour of #7081