Concurrency refactors and cleanups

[MisterDA]

While working on replacing winpthreads (the pthreads emulation layer on Windows) with direct use of the WinAPI concurrency primitives, I've gathered a few refactors and cleanups on systhreads and the runtime.
The end goal is to share common code between platforms using pthreads and Windows, and have platform specific code in different files. As it doesn't make much sense to have st_pthreads.h and st_posix.h I'm removing st_pthreads.h and inlining relevant code in either st_posix.h or st_win32.h, and introducing a new st_stubs.h for common code.
I'm opening this PR now because it mildly conflicts with #12410 and #13348, and I don't enjoy dealing with conflicts too much (but yay for git rerere), so better now than later? (of course if they get merged first, I'll rebase this PR).

[gasche]

I'm confused by this PR.

• Some commits are called "removed unexposed and unused foo_retcode typedef", but in fact the type is used in the codebase, apparently to track (as annotations) values that should be passed to sync_check_error. I would intuitively assume that the people who wrote this believed that this abstraction improves the code / reduces the chance for mistakes / makes it easier to check that the codebase is consistently checking for errors. Do you believe that this assumption does not hold anymore, and why? The only argument you hint at is that this typedef is not exposed in the user-facing APIs, I don't understand how that justifies removing it.

• The last commit ("Inline st_pthreads.h into st_posix.h and st_win32.h") appears to duplicate a large amount of code, including documentation comments, etc. (github reports it at 594 added lines for 386 removed lines). From a distance this looks like the opposite of a desirable refactoring to me. What am I missing?

As it doesn't make much sense to have st_pthreads.h and st_posix.h

My understanding is that st_pthreads.h contains common code that works with either pthreads (on Unixes) or winpthreads (on Windows), and st_posix.h contains two less-portable functions that deals with signals and is not implemented on Windows (st_win32.h only has placeholder definitions that fail at runtime).

[gasche]

Notes on the other commits (skimmed quickly):

• I'm happy with changes to use C99 booleans (I think we for now are trying not to use them in the user-facing API, but using them internally is good), feel free to submit a separate PR about it.

• I'm less happy with the commit that removes return; statement at the end of void-returning functions. The people who wrote the code apparently believed that it was good style, and your preference to remove them is just a difference in taste -- as far as I can tell there is no objective software-engineering justification for actively adding or removing return; statements at the end of void-returning functions. I don't want to waste our time looking at cosmetic changes that do not actually improve the codebase. Every change has a cost (in review time, rebase work, (not this time) regression risk, etc.), and "I like it better this way" is not enough benefit to offset the cost.

[MisterDA]

Thanks a lot for the review!

Some commits are called "removed unexposed and unused foo_retcode typedef", but in fact the type is used in the codebase, apparently to track (as annotations) values that should be passed to sync_check_error. I would intuitively assume that the people who wrote this believed that this abstraction improves the code / reduces the chance for mistakes / makes it easier to check that the codebase is consistently checking for errors. Do you believe that this assumption does not hold anymore, and why? The only argument you hint at is that this typedef is not exposed in the user-facing APIs, I don't understand how that justifies removing it.

I think the typedef would have been useful if the C compiler could somehow check conversions between the type alias and the type behind the typedef, a bit like private types in OCaml. It would also have been useful if we were declaring a custom error type. The caml_check_error or sync_check_error are not using the typedef. Reading the code I would have preferred for it to call a spade a spade and not rename the error type. If anything, maybe we didn't go far enough in the use of this abstraction.

The last commit ("Inline st_pthreads.h into st_posix.h and st_win32.h") appears to duplicate a large amount of code, including documentation comments, etc. (github reports it at 594 added lines for 386 removed lines). From a distance this looks like the opposite of a desirable refactoring to me. What am I missing?

In st_pthreads.h there's code using the pthread API, and code using our threads or runtime API. Code using pthreads needs to be ported on Windows, and the other code may be shared. Moving the code around eases the review for the follow-up PR, but I overlooked that this commit is too early for this PR.

As it doesn't make much sense to have st_pthreads.h and st_posix.h

My understanding is that st_pthreads.h contains common code that works with either pthreads (on Unixes) or winpthreads (on Windows), and st_posix.h contains two less-portable functions that deals with signals and is not implemented on Windows (st_win32.h only has placeholder definitions that fail at runtime).

Thanks for the explanation, I'll hold this commit for a while. It's not too bad to merge st_pthreads.h into st_posix.h, the "p" stands for "posix".

I'm less happy with the commit that removes return; statement at the end of void-returning functions.

IMO this is unnecessary code (early returns matter, but these?), inconsistent with the rest of the code base, and the commit reduces the line count.

[gasche]

You removed the commits I was grumpy about (thanks!).

I had a new look and they are overall fine, but I don't know about the last one ("extract common code that is not posix/pthreads"). I find the current (trunk) structure a bit complex already (st_pthreads.h is a "mixin" that both provides features and expects requirements from its includer), and the new layering you propose is even slightly more complex.

Two questions about this:

• One change in that commit is to move the tick_thread related code out of st_pthreads.h. Could you include it directly in st_stubs.c?
• It would be nice to also move the bt_lock_{acquire,release} stuff in st_stubs.c, but release depends on st_masterlock_waiters(m) which is in st_pthread.h. Could the prototype void st_bt_lock_release(st_masterlock *m) be changed to void st_bt_lock_release(bool other_threads_waiting) to cut the dependency cycle?

[MisterDA]

One change in that commit is to move the tick_thread related code out of st_pthreads.h. Could you include it directly in st_stubs.c?

Done.

It would be nice to also move the bt_lock_{acquire,release} stuff in st_stubs.c, but release depends on st_masterlock_waiters(m) which is in st_pthreads.h. Could the prototype void st_bt_lock_release(st_masterlock *m) be changed to void st_bt_lock_release(bool other_threads_waiting) to cut the dependency cycle?

It's also just possible to add prototypes for bt_lock_{acquire,release} and st_masterlock_waiters in st_pthreads.h and put the functions definitions in st_stubs.c. Would that be ok with you?

[gasche]

It's also just possible to add prototypes for bt_lock_{acquire,release} and st_masterlock_waiters in st_pthreads.h and put the functions definitions in st_stubs.c. Would that be ok with you?

I find this a bit complex. Maybe we should do the work of writing down the interface of both files, what they expect from the other file and what they provide?

If this is more work than you want to tackle, personally I would err on the side of keeping things in st_pthreads.h for now, we don't need to decouple everything in one go.

[MisterDA]

Maybe we should do the work of writing down the interface of both files, what they expect from the other file and what they provide?

I'll give it a go.

If this is more work than you want to tackle, personally I would err on the side of keeping things in st_pthreads.h for now, we don't need to decouple everything in one go.

I'm working on replacing winpthreads with Windows primitives. My idea was to extract functions that are not using pthreads/unix signals from st_pthreads.h and st_posix.h, so that st_win32.h can simply provide the same interface as st_pthreads.h and st_posix.h without duplicated code.

[dra27]

LGTM, once @gasche is also happy

[MisterDA]

Thanks for the reviews!

I've moved the declaration of the functions in st_subs.c and used a forward declaration of st_masterlock. I think the code is a bit clearer this way, I don't have a better idea on how to improve on that.

[gasche]

I don't have a better idea on how to improve on that.

Have you considered the suggestion I made earlier? It was as follows:

Could the prototype void st_bt_lock_release(st_masterlock *m) be changed to void st_bt_lock_release(bool other_threads_waiting) to cut the dependency cycle?

[MisterDA]

I don't have a better idea on how to improve on that.

Have you considered the suggestion I made earlier? It was as follows:

Could the prototype void st_bt_lock_release(st_masterlock *m) be changed to void st_bt_lock_release(bool other_threads_waiting) to cut the dependency cycle?

Indeed, I forgot this. It's really nice now, thanks!

[MisterDA]

I believe I've addressed all the comments.

[gasche]

You forgot to update the Changes entry to credit @dustanddreams' bug-catching review.

[gasche]

Merged, thanks!