Fix commandlog large-reply when using reply copy avoidance

[enjoy-binbin]

In #2078, we did not report large reply when copy avoidance is allowed.
This results in replies larger than 16384 not being recorded in the
commandlog large-reply. This 16384 is controlled by the hidden config
min-string-size-avoid-copy-reply.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.12%. Comparing base (ffdf222) to head (bf86728).
⚠️ Report is 117 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #2652      +/-   ##
============================================
- Coverage     72.23%   72.12%   -0.12%     
============================================
  Files           127      127              
  Lines         70935    70942       +7     
============================================
- Hits          51239    51164      -75     
- Misses        19696    19778      +82     

Files with missing lines	Coverage Δ
src/networking.c	88.31% <100.00%> (+0.12%)	⬆️

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[madolson]

@xbasel Will you get time to take a look at this?

[zuiderkwast]

@enjoy-binbin Shall we merge this and include it in 9.0.1? I can approve it fast if we need it.

[enjoy-binbin]

Shall we merge this and include it in 9.0.1? I can approve it fast if we need it.

Yes, we can do it. Although it is still lacking review, the code seems to be safe.

[roshkhatri]

I noticed that this commit is effecting the performance Get commands from 2.7M to 2.0M for 96 bytes data size, 9 IO-Threads and 10 Pipelining. this is about 25% drop

You can see more here

@enjoy-binbin Please can you take a look here

[enjoy-binbin]

odd, the code is quite simple, thanks for the report, i will take a look later.

[ranshid]

odd, the code is quite simple, thanks for the report, i will take a look later.

Isn;t this an issue since we access the string checking the length of it?

[madolson]

Isn;t this an issue since we access the string checking the length of it?

Yeah, this is why I had asked Basel to take a look to see if there was anything we can do about this PR. We are paying a large performance penalty by doing the memory dereference here.

[madolson]

Btw, Basel has since left Amazon so he probably won't follow up. This was a sticky issue we never really knew what to do about. The issue is we can't know the length of the SDS without de-referencing it at this point. We discussed doing it in the IO thread itself, but that would be a lot more work and require some mechanism to track the actual bytes written to the COB.

[enjoy-binbin]

ok, i see, sorry i didn't notice it before. Should we revert it before we have a solution?

This was a sticky issue we never really knew what to do about. The issue is we can't know the length of the SDS without de-referencing it at this point. We discussed doing it in the IO thread itself, but that would be a lot more work and require some mechanism to track the actual bytes written to the COB.

Yes, i also tried doing it in the io thread before, it is indeed hard to do it.

[zuiderkwast]

ok, i see, sorry i didn't notice it before. Should we revert it before we have a solution?

I don't think so. Correctness is more important than performance IMHO and I have already released this fix in 9.0.1.

When we have a fix, we can easily release another 9.0.2. Idea: Do this in the IO thread using an atomic variable or mutex.

[madolson]

I don't think so. Correctness is more important than performance IMHO and I have already released this fix in 9.0.1.

I don't agree with this. If someone was relying on the performance, it's effectively going to break their application. I think we shouldn't have released this 9.0.1.

We might be able to prefetch it when it's given to the io thread as well. If we use a mutex there will more likely be contention.

[zuiderkwast]

we shouldn't have released this 9.0.1

Fair enough. Yeah, if I'd known about the performance regression I had waited. But now that it's done, let's prioritize a fix and release it in 9.0.2 ASAP?

We might be able to prefetch it when it's given to the io thread as well

You mean prefetch earlier so when we access the length here, it's already prefetched? We already do that afaict: https://github.com/valkey-io/valkey/blob/9.0.1/src/memory_prefetch.c#L141

[zuiderkwast]

The c->net_output_bytes_curr_cmd accounting also affects CLUSTER SLOT-STATS, so that was probably another bug that was fixed by this PR.

[madolson]

Fair enough. Yeah, if I'd known about the performance regression I had waited. But now that it's done, let's prioritize a fix and release it in 9.0.2 ASAP?

Let's see if we can fix it first! We still have a problem. If we can't fix it we need to decide what to do now.

You mean prefetch earlier so when we access the length here, it's already prefetched? We already do that afaict: https://github.com/valkey-io/valkey/blob/9.0.1/src/memory_prefetch.c#L141

So the specific case that is failing is the 96 byte case. I wonder if that specific size the length is on a different cache line as the value ptr?

[zuiderkwast]

So the specific case that is failing is the 96 byte case. I wonder if that specific size the length is on a different cache line as the value ptr?

You mean we prefetch val->ptr but the size is located at 1-2 bytes before that? It seems unlinkely that the cache line boundary would be exactly there, because allocations of this size are normally 16B aligned and the sds header is only 3 bytes.

The sds pointer is pointing a few bytes into the allocation. Do you think it would make a difference to round the address down before prefetching, to prefetch the actual sds allocation's address? Like valkey_prefetch((void *)((uintptr_t)(val->ptr) & ~7))).