Require C11 _Atomics #490

adetunjii · 2024-05-12T00:54:46Z

Replaces custom atomics logic with C11 default atomics logic.
Drops "atomicvar_api" field from server info

Closes #485

codecov · 2024-05-13T03:15:12Z

Codecov Report

Attention: Patch coverage is 76.86567% with 31 lines in your changes are missing coverage. Please review.

Project coverage is 70.17%. Comparing base (1c55f3c) to head (6a73d02).

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #490      +/-   ##
============================================
- Coverage     70.19%   70.17%   -0.02%     
============================================
  Files           109      109              
  Lines         59905    59883      -22     
============================================
- Hits          42050    42024      -26     
- Misses        17855    17859       +4

Files	Coverage Δ
src/db.c	`88.27% <ø> (ø)`
src/evict.c	`97.34% <ø> (ø)`
src/functions.c	`95.58% <ø> (ø)`
src/rdb.c	`75.83% <100.00%> (+0.38%)`	⬆️
src/replication.c	`87.14% <100.00%> (ø)`
src/threads_mngr.c	`95.65% <100.00%> (-0.10%)`	⬇️
src/zmalloc.c	`84.64% <100.00%> (-0.07%)`	⬇️
src/module.c	`9.65% <50.00%> (+<0.01%)`	⬆️
src/aof.c	`80.05% <80.00%> (-0.02%)`	⬇️
src/networking.c	`85.21% <81.81%> (+0.21%)`	⬆️
... and 4 more

... and 10 files with indirect coverage changes

src/module.c

zuiderkwast

Looks good so far.

I think we can also wait for another changes which is in progress. It's is a change for I/O threads which will probably use atomic variables a lot.

My idea is that we replace all atomic{Get,Set,Incr} and also delete atomicvar.h in one PR just before Valkey 8 release candidate 1, when no new features will be added. Then, if some users have a problem when testing the release candidate, we can revert it before we release 8.0.

@madolson What do you think?

madolson · 2024-05-13T18:14:26Z

@zuiderkwast I guess i would prefer we do the C11 migration now. Then anyone who is building from unstable will be able to start relying on the new improvements. I would rather find out sooner that someone might still have build issues.

aiven-sal

You need to remove

valkey/src/zmalloc.c

Line 54 in dd2b27e

#include "atomicvar.h"

and replace

valkey/src/server.c

Line 5631 in dd2b27e

"atomicvar_api:%s\r\n", REDIS_ATOMIC_API,

with something meaningful (I'm not sure what that would be).
And then you can remove atomicvar.h completely.

src/bio.c

zuiderkwast · 2024-05-14T13:38:00Z

and replace

valkey/src/server.c

Line 5631 in dd2b27e

"atomicvar_api:%s\r\n", REDIS_ATOMIC_API,

with something meaningful (I'm not sure what that would be).

"c11-builtin" is what's used today when C11 atomics are used.

We can also consider dropping this info field.

adetunjii · 2024-05-14T14:18:26Z

and replace

valkey/src/server.c

Line 5631 in dd2b27e

"atomicvar_api:%s\r\n", REDIS_ATOMIC_API,

with something meaningful (I'm not sure what that would be).

"c11-builtin" is what's used today when C11 atomics are used.

We can also consider dropping this info field.

@zuiderkwast I think we should just drop it since we're making C11 the standard.

zuiderkwast · 2024-05-14T18:15:33Z

Ok, please mention it in the PR description that we're dropping the INFO field "atomicvar_api". It's a user-facing change.

Signed-off-by: adetunjii <[email protected]>

…ix/c11-atomics

aiven-sal

I think you can now rm src/atomicvar.h .
Thanks for taking the time to work on my comments!

src/server.c

Signed-off-by: Samuel Adetunji <[email protected]>

…ix/c11-atomics

aiven-sal

LGTM
Thank you!

zuiderkwast · 2024-05-16T16:48:43Z

We need to remove all CI jobs for CentOS 7. They fail because it doesn't support C11. Shall we change these jobs to CentOS 8? I think the point is to have one of the oldest OSes that's still supported. (CentOS 7 is EOL in June this year.)

adetunjii · 2024-05-17T10:56:46Z

We need to remove all CI jobs for CentOS 7. They fail because it doesn't support C11. Shall we change these jobs to CentOS 8? I think the point is to have one of the oldest OSes that's still supported. (CentOS 7 is EOL in June this year.)

Yeah. I think it's best to remove old CI jobs for CentoS 7. Let's fix that in another PR

zuiderkwast · 2024-05-17T11:33:18Z

I think it's best to remove old CI jobs for CentoS 7. Let's fix that in another PR

I don't like to merge a PR which breaks the CI. It affects the daily CI too. We can do it in a separate PR, but then we should merge that CI PR before we merge this PR.

adetunjii · 2024-05-17T11:34:32Z

Okay sure. I’ll have a look at it Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: Viktor Söderqvist ***@***.***> Sent: Friday, May 17, 2024 12:33:39 PM To: valkey-io/valkey ***@***.***> Cc: Samuel Adetunji ***@***.***>; Author ***@***.***> Subject: Re: [valkey-io/valkey] Introduce C11 _Atomics (PR #490) I think it's best to remove old CI jobs for CentoS 7. Let's fix that in another PR I don't like to merge a PR which breaks the CI. It affects the daily CI too. We can do it in a separate PR, but then we should merge that CI PR before we merge this PR. — Reply to this email directly, view it on GitHub<#490 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AKXTHW3KXAVQZ235T2KQEZDZCXTJHAVCNFSM6AAAAABHSLAAJSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJXGM4TQOJUGM>. You are receiving this because you authored the thread.Message ID: ***@***.***>

madolson · 2024-05-19T19:22:01Z

Shall we change these jobs to CentOS 8?

CentOS 8 is already EoL, it never got long term support from RHEL from the looks of it. CentOS also doesn't exist anymore, it was evolved into CentOS stream. We could try AlmaLinux 9 or CentOS stream 9 as a replacement. I pinged some folks, but my initial guess is to move to CentOS stream.

EDIT: Ideally we should have a matrix and do CentOS stream 9 and AlmaLinux 8. AlmaLinux 8 will be maintained until 2029, so it seems like a great replacement for our CentOS 7 test.

zuiderkwast

I've looked though this diff now. Looks good, but I think I spotted two typos.

src/networking.c

zuiderkwast

LGTM.

Don't merge until we have replaced CentOS 7 in CI jobs.

madolson · 2024-05-21T20:47:48Z

I documented what we should do about centos 7 here: #527, in case someone else has time to pick it up.

Signed-off-by: Samuel Adetunji <[email protected]>

zuiderkwast · 2024-05-26T14:30:54Z

Centos7 is still in the CI in this branch. I don't know how it happened. Did you merge latest unstable?

You can use merge commits. No need to rebase and force-push. We'll squash-merge it anyway.

Signed-off-by: Samuel Adetunji <[email protected]>

adetunjii · 2024-05-26T14:37:46Z

Centos7 is still in the CI in this branch. I don't know how it happened. Did you merge latest unstable?

You can use merge commits. No need to rebase and force-push. We'll squash-merge it anyway.

It should be fine now

adetunjii · 2024-05-26T14:57:43Z

@zuiderkwast when can this be merged?

zuiderkwast · 2024-05-26T16:32:11Z

@zuiderkwast when can this be merged?

Probably tomorrow. People are not working today.

zuiderkwast · 2024-05-26T16:41:42Z

Actually we discussed it now and it seems fine to merge. 🚀

madolson requested a review from zuiderkwast May 12, 2024 16:29

aiven-sal reviewed May 13, 2024

View reviewed changes

src/module.c Outdated Show resolved Hide resolved

zuiderkwast reviewed May 13, 2024

View reviewed changes

adetunjii requested review from zuiderkwast and aiven-sal May 14, 2024 01:34

aiven-sal reviewed May 14, 2024

View reviewed changes

src/bio.c Show resolved Hide resolved

zuiderkwast added the release-notes This issue should get a line item in the release notes label May 14, 2024

adetunjii requested a review from aiven-sal May 15, 2024 00:16

adetunjii force-pushed the fix/c11-atomics branch 5 times, most recently from 7e8380c to 8b7cfa0 Compare May 15, 2024 01:08

adetunjii and others added 3 commits May 15, 2024 02:09

Introduce C11 _Atomics

883573d

Signed-off-by: adetunjii <[email protected]>

replace atomicvar header file

32dc844

Signed-off-by: adetunjii <[email protected]>

drop atomic_var field from server info

08ce3ee

Signed-off-by: adetunjii <[email protected]>

adetunjii force-pushed the fix/c11-atomics branch from 8b7cfa0 to 08ce3ee Compare May 15, 2024 01:11

Merge branch 'unstable' of https://github.com/adetunjii/valkey into f…

98407ce

…ix/c11-atomics

aiven-sal reviewed May 15, 2024

View reviewed changes

src/server.c Outdated Show resolved Hide resolved

remove atomicvar header file

14ba161

Signed-off-by: Samuel Adetunji <[email protected]>

adetunjii requested a review from aiven-sal May 15, 2024 23:45

Merge branch 'unstable' of https://github.com/adetunjii/valkey into f…

d3f984a

…ix/c11-atomics

aiven-sal approved these changes May 16, 2024

View reviewed changes

zuiderkwast reviewed May 20, 2024

View reviewed changes

src/networking.c Show resolved Hide resolved

src/networking.c Show resolved Hide resolved

zuiderkwast approved these changes May 21, 2024

View reviewed changes

aiven-sal mentioned this pull request May 22, 2024

Upgrade centos 7 to centos stream 9 #531

Closed

adetunjii requested review from aiven-sal and zuiderkwast May 26, 2024 13:09

fix merge conflicts

5489fc2

Signed-off-by: Samuel Adetunji <[email protected]>

adetunjii force-pushed the fix/c11-atomics branch from 66dc020 to 5489fc2 Compare May 26, 2024 13:40

fix merge conflicts

6a73d02

Signed-off-by: Samuel Adetunji <[email protected]>

zuiderkwast merged commit 5d0f4bc into valkey-io:unstable May 26, 2024
17 checks passed

zuiderkwast changed the title ~~Introduce C11 _Atomics~~ Require C11 _Atomics May 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require C11 _Atomics #490

Require C11 _Atomics #490

adetunjii commented May 12, 2024 •

edited

Loading

codecov bot commented May 13, 2024 •

edited

Loading

zuiderkwast left a comment

madolson commented May 13, 2024

aiven-sal left a comment

zuiderkwast commented May 14, 2024 •

edited

Loading

adetunjii commented May 14, 2024

zuiderkwast commented May 14, 2024

aiven-sal left a comment

aiven-sal left a comment

zuiderkwast commented May 16, 2024

adetunjii commented May 17, 2024

zuiderkwast commented May 17, 2024

adetunjii commented May 17, 2024 via email

madolson commented May 19, 2024 •

edited

Loading

zuiderkwast left a comment

zuiderkwast left a comment

madolson commented May 21, 2024

zuiderkwast commented May 26, 2024

adetunjii commented May 26, 2024

adetunjii commented May 26, 2024

zuiderkwast commented May 26, 2024

zuiderkwast commented May 26, 2024

Require C11 _Atomics #490

Require C11 _Atomics #490

Conversation

adetunjii commented May 12, 2024 • edited Loading

codecov bot commented May 13, 2024 • edited Loading

Codecov Report

zuiderkwast left a comment

Choose a reason for hiding this comment

madolson commented May 13, 2024

aiven-sal left a comment

Choose a reason for hiding this comment

zuiderkwast commented May 14, 2024 • edited Loading

adetunjii commented May 14, 2024

zuiderkwast commented May 14, 2024

aiven-sal left a comment

Choose a reason for hiding this comment

aiven-sal left a comment

Choose a reason for hiding this comment

zuiderkwast commented May 16, 2024

adetunjii commented May 17, 2024

zuiderkwast commented May 17, 2024

adetunjii commented May 17, 2024 via email

madolson commented May 19, 2024 • edited Loading

zuiderkwast left a comment

Choose a reason for hiding this comment

zuiderkwast left a comment

Choose a reason for hiding this comment

madolson commented May 21, 2024

zuiderkwast commented May 26, 2024

adetunjii commented May 26, 2024

adetunjii commented May 26, 2024

zuiderkwast commented May 26, 2024

zuiderkwast commented May 26, 2024

adetunjii commented May 12, 2024 •

edited

Loading

codecov bot commented May 13, 2024 •

edited

Loading

zuiderkwast commented May 14, 2024 •

edited

Loading

madolson commented May 19, 2024 •

edited

Loading