What if we do relaxed load first and then only acquire on successful claim? I believe that would help under high contention
UPD: Tried locally, was not beneficial for SET load

Why do we need alignment here? Wouldn't it be better to multiple cells in one cache line? Or we can even separate hot/colder paths here, embed _Alignas(CACHE_LINE_SIZE) atomic_size_t *sequences; into spmcQueue and separately add _Alignas(CACHE_LINE_SIZE) void **data; WDYT?

My understanding is the intention behind this was reduce cache line contention. I can dig in further, but I suspect that reverting this can result in more CPU cache invalidation and can degrade performance.

I think your concern is valid, if we are losing 48 bytes per cell to internal fragmentation and the queue size is set to 4096, we are losing nearly 200kb to internal fragmentation.

If this overhead cost is too high, we can evaluate the performance trade off. The memory is only allocated if the feature is turned on.

unpack_job() expects a void ** for the output pointer, but this call passes (void *)&c, which is a void * and will generate an incompatible-pointer-type warning (and is easy to misuse). Pass the address with the correct type (e.g., cast to void **) to make the intent and typing safe.

now is a long long (milliseconds), but last_sample_time is declared as size_t. The subtraction now - last_sample_time mixes signed/unsigned types and can behave unexpectedly (or at least be compiler-warning-prone) on some platforms. Prefer using long long/mstime_t consistently for timestamps here.

switching datatype of last_sample_time and now to mstime_t

I think there is a broad preference to avoid instantaneous metrics. There was also another PR merged for main thread CPU, so not sure we just need IO queue size.

So we know practically have two SPSC (mutexQueue) and this. I understand they have slightly different guarantees, but it would be preferably to only have 1. @JimB123 thoughts if we can just use one?

MutexQueue is MPMC. It's designed to be general purpose.

This queue is custom built SPSC for this purpose only. It's not reusable. This is designed to be fast for this one purpose only. AFAIK, there have been no performance comparisons, but I would expect that this custom implementation would be better for this custom purpose.

I think we have a few options:

1. Use mutexQueue instead - it's designed to be multi-purpose. But it may reduce performance for this case.
2. Try to re-design this new queue to be a faster lockless queue with its own unique API. This might make it more reusable, but also might impact its usefulness for this specific use case.
3. Keep it separate, but ensure that the naming conveys that it's custom built for one purpose. Looking at the naming (io_queues.h) it seems like maybe this is already done.

It's really hard to tell if this is just premature optimization or not. For example, coding it all in a header file doesn't really bring value now that we have LTO. It might just cause code bloat.

The BIO code is also SPSC, so it seems like we might be using the wrong datatype for that then.

This queue is custom built SPSC for this purpose only. It's not reusable. This is designed to be fast for this one purpose only.

It's a bold statement. 😄

In the mutexqueue, all operations are protected by a single mutex. That's what makes it MPMC. It's also resizable. It never rejects an insert.

That design seems optimal for async work like lazyfree and forkless, but suboptimal for io-threads communication where we need low latency though the queue.

I see these io-queues as mostly generic, just fixed size. We can say they're optimized for low latency.

I don't see a problem having both in the repo. (But if we really want, I guess we could use a fixed size queue for lazyfree. When it's full, the main thread could do the freeing by itself. For forkless, the main threads would have to wait a bit if the queue gets full. I don't if that would be very bad or not. An aspect is that the memory usage gets bounded.)

// bio.c
#define SPSC_QUEUE_SIZE 1024
#include "queues.h"

// queues.h
#ifndef SPSC_QUEUE_SIZE
#define SPSC_QUEUE_SIZE 4096
#endif

What exactly are end users supposed to do about this metric?

Agree, we probably don't need to expose these.

Discussed in core team. Probably remove the active_io_threads_num, since it changes so much. Then move these to the info_debug section.

This isn't actually the memory usage we're tracking, it's the value we're computing int he cron. I wonder if it would be a bit simpler to just to zero out this clients contribution to the memory usage bucket. That would still prevent the looping here.

Generally we just have text comments, not doxygen style comments.

There is implicit ordering between these two, since io_threads uses inMainThread(). It would be nice to make it so that io_queues was independent so this doesn't break later.

forward declaration in io_queues.h would solve this problem

This code is now also in fifo.c, would be nice to move this to a helper. Also by convention it would be packJob. (Although I would prefer tagPointer() and untagPointer()).

Renaming this function to packJob

Regarding moving this to a helper,
looks like @JimB123's fifo mutexQueue uses union based pointer tagging to store the first or last element index in the block.
And, this PR uses this function to tag any generic pointer like client, argv, etc.

But, to promote general reusability, this may be moved to any header file that would cause less header bloat.

I'm a big fan of functional units with clean APIs.

Tagging pointers (using the extra bits for flags or other purposes) is a pretty low level operation. My expectation is that this should not be exposed beyond the unit. It should be internal, in the C file, and static.

That's exactly what I see above. A small static routine that's internal only, and not exposed. The same is true for the pointer tagging in mutexQueue.

Given that, I don't feel strongly that there is a need to organize common helper functions.

If we did want to create helper functions, the naming and use can get challenging. For example, names like "packJob" would be inappropriate. Also, the use cases themselves can be different - sometimes individual bits are used as flags. Sometimes bits are used together as an integral value. Combinations, etc.

Having private, internal, routines likely allows for more use-case driven naming - better readability.

I don't really understand the motivation behind the 30% here. Don't we want to be much closer to 100% to start the second thread? Maybe include some documentation here about how these numbers were picked, I didn't see anything in the top level comment either.

CPU SYS is kernel CPU time I believe. User space and kernel space CPU time is tracked separately.

I posted another comment about this, but I guess if we use the new used_active_time_main_thread metric instead, it can be more reliable for ignition. That one simply measures how long we work vs wait for work in plain monotonic time, so when this one is close to 100% it means the main thread is spending no time waiting for more work, so we can ignite.

There is a new metric that's just using monotonic time to measure the time the main thread is trying to work vs waiting for work: #2931. When IO threads are disabled, this is essentially the total time minus the time spent in blocking epoll.

We could use that (possibly transformed into an instantaneous metric) instead of CPU usage for ignition. It is cheaper and I guess it can also be more reliable. CPU usage can be low even if the thread is busy, if we're waiting for blocking IO, if there is preemption happening, DEBUG SLEEP and some other situations.

I mean we store it as an instantaneous metric internally if you need it over the last 1.6 seconds, but we don't need to expose a new INFO field for it.