client struct: lazy init components and optimize struct layout

[uriyage]

Refactor client structure to use modular data components

Current State

The client structure allocates memory for replication / pubsub / multi-keys / module / blocked data for every client, despite these features being used by only a small subset of clients. In addition the current field layout in the client struct is suboptimal, with poor alignment and unnecessary padding between fields, leading to a larger than necessary memory footprint of 896 bytes per client. Furthermore, fields that are frequently accessed together during operations are scattered throughout the struct, resulting in poor cache locality.

This PR's Change

1. Lazy Initialization

• Components are only allocated when first used:
  • PubSubData: Created on first SUBSCRIBE/PUBLISH operation
  • ReplicationData: Initialized only for replica connections
  • ModuleData: Allocated when module interaction begins
  • BlockingState: Created when first blocking command is issued
  • MultiState: Initialized on MULTI command

2. Memory Layout Optimization:

   • Grouped related fields for better locality
   • Moved rarely accessed fields (e.g., client->name) to struct end
   • Optimized field alignment to eliminate padding

3. Additional changes:

   • Moved watched_keys to be static allocated in the mstate struct
   • Relocated replication init logic to replication.c

Key Benefits

• Efficient Memory Usage:
• 45% smaller base client structure - Basic clients now use 528 bytes (down from 896).
• Better memory locality for related operations
• Performance improvement in high throughput scenarios. No performance regressions in other cases.

Performance Impact

Tested with 650 clients and 512 bytes values.

Single Thread Performance

Operation	Dataset	New (ops/sec)	Old (ops/sec)	Change %
SET	1 key	261,799	258,261	+1.37%
SET	3M keys	209,134	~209,000	~0%
GET	1 key	281,564	277,965	+1.29%
GET	3M keys	231,158	228,410	+1.20%

8 IO Threads Performance

Operation	Dataset	New (ops/sec)	Old (ops/sec)	Change %
SET	1 key	1,331,578	1,331,626	-0.00%
SET	3M keys	1,254,441	1,152,645	+8.83%
GET	1 key	1,293,149	1,289,503	+0.28%
GET	3M keys	1,152,898	1,101,791	+4.64%

Pipeline Performance (3M keys)

Operation	Pipeline Size	New (ops/sec)	Old (ops/sec)	Change %
SET	10	548,964	538,498	+1.94%
SET	20	606,148	594,872	+1.89%
SET	30	631,122	616,606	+2.35%
GET	10	628,482	624,166	+0.69%
GET	20	687,371	681,659	+0.84%
GET	30	725,855	721,102	+0.66%

Observations:

1. Single-threaded operations show consistent improvements (1-1.4%)
2. Multi-threaded performance shows significant gains for large datasets:
   • SET with 3M keys: +8.83% improvement
   • GET with 3M keys: +4.64% improvement
3. Pipeline operations show consistent improvements:
   • SET operations: +1.89% to +2.35%
   • GET operations: +0.66% to +0.84%
4. No performance regressions observed in any test scenario

Related issue:#761

[codecov]

Codecov Report

Attention: Patch coverage is 88.08594% with 61 lines in your changes missing coverage. Please review.

Project coverage is 70.79%. Comparing base (f20d629) to head (ff1b620).
Report is 30 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/module.c	4.08%	47 Missing ⚠️
src/replication.c	95.04%	10 Missing ⚠️
src/rdb.c	50.00%	2 Missing ⚠️
src/blocked.c	98.38%	1 Missing ⚠️
src/networking.c	98.36%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1405      +/-   ##
============================================
- Coverage     70.81%   70.79%   -0.02%     
============================================
  Files           118      119       +1     
  Lines         63561    64728    +1167     
============================================
+ Hits          45010    45826     +816     
- Misses        18551    18902     +351     

Files with missing lines	Coverage Δ
src/acl.c	88.77% <ø> (-0.06%)	⬇️
src/aof.c	80.20% <100.00%> (+0.07%)	⬆️
src/cluster.c	88.65% <100.00%> (+0.14%)	⬆️
src/cluster_legacy.c	86.76% <ø> (+0.06%)	⬆️
src/multi.c	97.30% <100.00%> (+0.87%)	⬆️
src/pubsub.c	96.82% <100.00%> (-0.23%)	⬇️
src/script.c	89.06% <100.00%> (+1.41%)	⬆️
src/server.c	87.45% <100.00%> (+0.06%)	⬆️
src/server.h	100.00% <ø> (ø)
src/timeout.c	90.00% <100.00%> (ø)
... and 6 more

... and 46 files with indirect coverage changes

[zuiderkwast]

• 45% smaller base client structure - Basic clients now use 528 bytes (down from 896).

Amazing!

[zuiderkwast]

Very good! It's a large diff but most of the changed lines are trivial changes. I can't verify everything so I have to trust you and the tests for the correctness.

I noticed in many helper functions, we're using these client sub-structs, for example c->mstate->something, without checking that c->mstate exists. It should always exist in these cases so it's not an error, but would it make sense to add serverAssert(c->mstate != NULL) in all these places?

If we have code coverage by tests, we will find these errors anyway, so IMHO it doens't matter too match if it's a null-pointer crash or an assert. What is your reasoning about this?

[zuiderkwast]

Just a few nits now.

[ranshid]

I think the overall change looks O.K, but some cases are not 100% trivial IMO and the change impacts almost all logical flows. I would suggest we run the daily tests to verify this change.

[ranshid]

Not that I see a real concern here, but we might think this should have been under assert check which would have forced the user to make sure he freeClientReplicationData before init again (if that case ever exists)

[ranshid]

So in this case we perform the actions of freeClient (eg all the dictReleases) when we unlink the client. Not sure it is problematic, but it is a change in order which is very basic.