Replace client flags to bitfield by artikell · Pull Request #614 · valkey-io/valkey

artikell · 2024-06-09T05:18:36Z

This is a complete version of PR #592, with the core points being:
-Replacing bitflags with flags
-Supports a clientFlags type for passing parameters, such as acceptCommonHandler
-Retained flags for initialization

codecov · 2024-06-09T06:00:09Z

Codecov Report

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

Project coverage is 70.30%. Comparing base (7415a57) to head (fac1adb).

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #614      +/-   ##
============================================
+ Coverage     70.28%   70.30%   +0.02%     
============================================
  Files           111      111              
  Lines         60215    60242      +27     
============================================
+ Hits          42320    42353      +33     
+ Misses        17895    17889       -6

Files	Coverage Δ
src/acl.c	`88.89% <100.00%> (-0.01%)`	⬇️
src/aof.c	`80.00% <100.00%> (-0.06%)`	⬇️
src/cluster.c	`88.26% <100.00%> (ø)`
src/cluster_legacy.c	`86.05% <100.00%> (+0.16%)`	⬆️
src/db.c	`88.40% <100.00%> (-0.01%)`	⬇️
src/functions.c	`95.59% <100.00%> (+<0.01%)`	⬆️
src/logreqres.c	`72.72% <ø> (ø)`
src/multi.c	`97.10% <100.00%> (+0.02%)`	⬆️
src/object.c	`78.57% <100.00%> (ø)`
src/script.c	`86.61% <100.00%> (ø)`
... and 21 more

... and 5 files with indirect coverage changes

madolson

A couple of a high level things before diving deep into all the usage. Mostly looked good though.

madolson · 2024-06-09T14:09:31Z

     * even if no sort order is requested, so they remain stable across
     * scripting and replication. */
-    if (dontsort && sortval->type == OBJ_SET && (storekey || c->flags & CLIENT_SCRIPT)) {
+    if (dontsort && sortval->type == OBJ_SET && (storekey || c->script)) {


Suggested change

if (dontsort && sortval->type == OBJ_SET && (storekey || c->script)) {

if (dontsort && sortval->type == OBJ_SET && (storekey || c->bitflags.script)) {

The refactor that removes the "flag" makes many of these variables quite a bit harder to read, since c->script seems more likely to indicate the script it's running than if it's a script client.

I don't like c->bitflags.script. The bitflags part doesn't bring additional value and makes the code hard to write/read IMO. Can we consider c->is_script_client instead?

I don't like c->bitflags.script. The bitflags part doesn't bring additional value and makes the code hard to write/read IMO. Can we consider c->is_script_client instead?

I think is_script_client only makes more sense if that is the only accessor method, but that isn't the case. bitflags makes it clear it is a wrapper structure around a bit of lower level flags. (If space wasn't a concern bitflags.is_script_client also seems ok to me).

It's an anonymous union, nice!

I can agree with Ping that bitflags (as in c->bitflags.script) looks a bit ugly. It exposes too much about the internal representation. Can we find another name?

c->flags is all flags, but with the bitfield we access one flag at a time, so how about something singular or boolean-like....

c->flag.script

c->flagged_as.script

c->is.script_client

@PingXie Can you chime in here, I don't have a strong opinion and am happy to defer to whatever you think is clear.

c->flag.script

I like this one.

Originally I was hoping to just go with an anonymous struct but now I notice that we actually need a named struct in places like acceptCommonHandler.

I think we are aligned directionally. @artikell , do you mind updating bitflags to flag? I will work with you to get this PR into Valkey 8.0

Sure, I think we will come this week to complete these processes.

artikell · 2024-06-09T18:23:42Z

A couple of a high level things before diving deep into all the usage. Mostly looked good though.

I agree, so I have fixed the known problem. Let's see if we really need to make such modifications in the future.

The usage of bitfields is more user-friendly for debugging and code reading.

However, for some batch operations, such as clearing data, passing between functions, and batch assignment, they can be quite cumbersome.

Signed-off-by: artikell <739609084@qq.com> Co-authored-by: Ping Xie <pingxie@google.com> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com> Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

PingXie

Thanks @artikell! Overall LGTM. Only 2 nitpicks

I prefer !c->flag.foo to !(c->flag.foo)
what do you think about flags vs raw_flag?

Signed-off-by: artikell <739609084@qq.com>

artikell · 2024-06-30T10:55:38Z

Thanks @artikell! Overall LGTM. Only 2 nitpicks

I prefer !c->flag.foo to !(c->flag.foo)

what do you think about flags vs raw_flag?

I hesitated about modifying the parentheses. Avoid increasing the burden on reviewers. But this is correct, I have made all the modifications.

I tend to lean towards raw_flag, which can distinguish it from flag. If using flags, it is easy to confuse. And I think raw can be expressed as an unorganized space.

zuiderkwast · 2024-06-30T21:17:34Z

-#define CLIENT_MONITOR (1 << 2)           /* This client is a replica monitor, see MONITOR */
-#define CLIENT_MULTI (1 << 3)             /* This client is in a MULTI context */
-#define CLIENT_BLOCKED (1 << 4)           /* The client is waiting in a blocking operation */
-#define CLIENT_DIRTY_CAS (1 << 5)         /* Watched keys modified. EXEC will fail. */


We lost these comments in this refactoring. Some of them may be useful I think. We can add them at the bit field some other time.

Let's add them now :-) #718

The bits should be 10, it causes ClientFlags to consume 8 more bytes now. Introduced in valkey-io#614. Signed-off-by: Binbin <binloveplay1314@qq.com>

The bits should be 10, it causes ClientFlags to consume 8 more bytes now. Introduced in #614. Signed-off-by: Binbin <binloveplay1314@qq.com>

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>

artikell force-pushed the switch_bitfield branch from 8d57a82 to f504952 Compare June 9, 2024 05:48

madolson reviewed Jun 9, 2024

View reviewed changes

artikell force-pushed the switch_bitfield branch from 45aa813 to 8435c10 Compare June 9, 2024 18:03

artikell mentioned this pull request Jun 9, 2024

Moving client->authenticated to a flag instead of an int #592

Merged

madolson requested a review from PingXie June 12, 2024 15:33

artikell force-pushed the switch_bitfield branch 2 times, most recently from 7260eb4 to 8822d68 Compare June 30, 2024 06:52

Replace client flags to bitfield

c224390

Signed-off-by: artikell <739609084@qq.com> Co-authored-by: Ping Xie <pingxie@google.com> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com> Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

artikell force-pushed the switch_bitfield branch from 8822d68 to c224390 Compare June 30, 2024 07:12

PingXie reviewed Jun 30, 2024

View reviewed changes

Comment thread src/blocked.c Outdated

Comment thread src/blocked.c Outdated

Comment thread src/blocked.c Outdated

Comment thread src/server.h

Remove excess parentheses

fac1adb

Signed-off-by: artikell <739609084@qq.com>

PingXie approved these changes Jun 30, 2024

View reviewed changes

PingXie merged commit e4c1f6d into valkey-io:unstable Jun 30, 2024

zuiderkwast reviewed Jun 30, 2024

View reviewed changes

enjoy-binbin added a commit to enjoy-binbin/valkey that referenced this pull request Jul 2, 2024

Fix wrong reserved bits in ClientFlags

7dc534e

The bits should be 10, it causes ClientFlags to consume 8 more bytes now. Introduced in valkey-io#614. Signed-off-by: Binbin <binloveplay1314@qq.com>

enjoy-binbin mentioned this pull request Jul 2, 2024

Fix wrong reserved bits in ClientFlags #729

Merged

enjoy-binbin added a commit that referenced this pull request Jul 2, 2024

Fix wrong reserved bits in ClientFlags (#729)

0cc16d0

The bits should be 10, it causes ClientFlags to consume 8 more bytes now. Introduced in #614. Signed-off-by: Binbin <binloveplay1314@qq.com>

pizhenwei added a commit to pizhenwei/valkey that referenced this pull request Jul 3, 2024

Compat e4c1f6d "Replace client flags to bitfield (valkey-io#614)"

cb332cf

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>

	if (dontsort && sortval->type == OBJ_SET && (storekey \|\| c->script)) {
	if (dontsort && sortval->type == OBJ_SET && (storekey \|\| c->bitflags.script)) {

Conversation

artikell commented Jun 9, 2024

Uh oh!

codecov Bot commented Jun 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

madolson Jun 9, 2024

Choose a reason for hiding this comment

Uh oh!

PingXie Jun 9, 2024

Choose a reason for hiding this comment

Uh oh!

madolson Jun 10, 2024

Choose a reason for hiding this comment

Uh oh!

zuiderkwast Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

madolson Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

PingXie Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artikell Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

artikell commented Jun 9, 2024

Uh oh!

PingXie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

artikell commented Jun 30, 2024

Uh oh!

zuiderkwast Jun 30, 2024

Choose a reason for hiding this comment

Uh oh!

PingXie Jun 30, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Jun 9, 2024 •

edited

Loading

zuiderkwast Jun 25, 2024 •

edited

Loading

PingXie Jun 26, 2024 •

edited

Loading