Skip to content

signal: atomic access #1363

Closed
yogo1212 wants to merge 6 commits intolibevent:masterfrom
yogo1212:signal_windows
Closed

signal: atomic access #1363
yogo1212 wants to merge 6 commits intolibevent:masterfrom
yogo1212:signal_windows

Conversation

@yogo1212
Copy link
Copy Markdown
Contributor

@yogo1212 yogo1212 commented Oct 26, 2022

building on #1105, implement a workaround for incomplete stdatomics support on windows

@yogo1212
Copy link
Copy Markdown
Contributor Author

yogo1212 commented Oct 31, 2022

pew.. this predefined macro stuff is... tedious..

@yogo1212
Copy link
Copy Markdown
Contributor Author

yogo1212 commented Nov 2, 2022

gotta say: msvc? not inspiring.
not supporting stdatomics is one thing but also not including any macros that would allow making a clean workaround? grrr!

_M_AMD64 and _M_ARM64 feel kind of weak, so i'll do runtime checks that will hopefully be optimised.

@yogo1212 yogo1212 marked this pull request as ready for review November 2, 2022 13:35
@yogo1212
Copy link
Copy Markdown
Contributor Author

yogo1212 commented Nov 2, 2022

@azat this should be it

"programming in 2 minute chunks over the course of a week"

azat
azat reviewed Nov 12, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@azat azat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, please take a look at the comments.

signal.c Outdated
intptr_t comparand = *p;
do {
x = comparand;
if (sizeof(*p) == 8)
Copy link
Copy Markdown
Member

@azat azat Nov 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to detect this at configuration time (cmake/autotools):

  • for cmake there is already

    libevent/CMakeLists.txt

    Line 709 in 9e34693

    CHECK_TYPE_SIZE("uintptr_t" EVENT__HAVE_UINTPTR_T)
  • but it is not enough since it uses #cmakedefine, while it should use @EVENT__HAVE_UINTPTR_T@ here -

    libevent/event-config.h.cmake

    Line 444 in 9e34693

    #cmakedefine EVENT__HAVE_UINTPTR_T 1
  • and for autotools -

    libevent/configure.ac

    Line 551 in 9e34693

    AC_CHECK_TYPES([uint64_t, uint32_t, uint16_t, uint8_t, uintptr_t], , ,
    seems that AC_CHECK_SIZEOF should be used for uintptr_t

signal.c Outdated
do {
x = comparand;
if (sizeof(*p) == 8)
comparand = _InterlockedCompareExchange64(p, x, comparand);
Copy link
Copy Markdown
Member

@azat azat Nov 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this (including loop) be replaced with _InterlockedCompareExchange(p, 0, 0) ?

P.S. for x86_64 we don't even need to use atomic_store and can simply use volatile, but to keep thins more reliable, I'm suggesting this.

Copy link
Copy Markdown
Contributor Author

@yogo1212 yogo1212 Nov 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the default ordering for atomic_ operations is "sequentially consistent" to ensure consistent behaviour when protecting locks (from acquire to release).
it's probably fine to go without but it shouldn't be called atomic_load anymore.

imo, the "proper" solution is to add an abstraction that ensures at least "relaxed" ordering.
it would be implemented using atomic_ or _Interlocked and the atomic_ variant would just be slight overkill.

Copy link
Copy Markdown
Member

@azat azat Nov 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be great to avoid overhead here, since then, we may consider using it to resolve #777 (it is more hot place than this one)
Are you willing to make proper primitives or it is better to merge this as-is?

@azat azat mentioned this pull request Nov 12, 2022
Open
@azat
Copy link
Copy Markdown
Member

azat commented Nov 14, 2022

By the way, this is very willing patch!

@yogo1212
Copy link
Copy Markdown
Contributor Author

yogo1212 commented Nov 15, 2022

i've had a meal, slept for a few hours, had another meal, brought a car to the mechanic, walked back, ate more, and these checks still aren't done :-D

@azat
Copy link
Copy Markdown
Member

azat commented Nov 15, 2022

i've had a meal, slept for a few hours, had another meal, brought a car to the mechanic, walked back, ate more, and these checks still aren't done :-D

Sadly, but we are using public workers, and it seems that there is one global pool for project, so one queue for all pull requests and master branch.
Plus we have tests for netbsd/freebsd/osx, and all of them uses osx machines, and that osx machine is quite a deficit...

@yogo1212
Copy link
Copy Markdown
Contributor Author

yogo1212 commented Nov 16, 2022 •
edited
Loading

no problem - i just thought it to be funny 😛

i'm pushing my changes to event.c and so into this branch.

unrelated:

Untracked files:
  (use "git add <file>..." to include in what will be committed)
        sample/ws-chat-server

issue with gitignore?

@yogo1212 yogo1212 force-pushed the signal_windows branch 2 times, most recently from 2a5eba1 to e98bbc5 Compare November 16, 2022 18:10
@widgetii widgetii mentioned this pull request Nov 16, 2022
Merged
@azat azat closed this in #1376 Nov 16, 2022
@yogo1212
Copy link
Copy Markdown
Contributor Author

yogo1212 commented Nov 17, 2022

@azat accidental close?

@azat
Copy link
Copy Markdown
Member

azat commented Nov 17, 2022

Ugh, sorry, that was about a comment not the issue.
And seems that the branch is gone or something similar, can you please re submit?

@azat
Copy link
Copy Markdown
Member

azat commented Nov 17, 2022

I would love to preserve all the discussion but github does not allow to reopen if you force pushed to the branch. Sigh.

@yogo1212 yogo1212 mentioned this pull request Nov 19, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@azat azat azat left review comments

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@yogo1212 @azat @htuch