Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Atomic destructors #12631

Open
mratsim opened this issue Nov 9, 2019 · 3 comments
Open

Atomic destructors #12631

mratsim opened this issue Nov 9, 2019 · 3 comments

Comments

@mratsim
Copy link
Collaborator

mratsim commented Nov 9, 2019

The move and wasMoved procs need an escape hatch for Atomics.

Today we can use the {.nodestroy.} pragma, originally designed to avoid deep recursion.

However, it would be very easy to use an atomic and forget about no destroy.

Example

For example here is a single-producer single-consumer channel that can only buffer a pointer

import atomics
type
  Channel = object
    # Single-Producer Single-Consumer
    # Only one thread can call trySend
    # and only another can call tryRecv
    pad: [128 - sizeof(pointer), byte] # Padded to avoid false sharing when we have an array of channels
    buffer: pointer

proc trySend(chan: var Channel, msg: sink pointer): bool =
  let p = chan.buffer.load(moAcquire)
  if not p.isNil:
    return false
  `=sink`(chan.buffer, src)
  return true

func tryRecv*(chan: var ChannelRaw, dst: var pointer): bool {.inline, nodestroy.} =
  let p = chan.buffer.load(moAcquire)
  if p.isNil:
    return false
  dst = chan.buffer # atomicMove could replace the 2 lines
  chan.buffer.store(nil, moRelease)
  return true

In the tryRecv function, using move instead would be equivalent to

  dst = chan.buffer
  chan.buffer = nil

This is not the same as the compiler could reorder the nil statement and violates the moRelease requirements (not here due to data dependencies).

Also while there would not be any different instructions emitted on X86 due to the strong memory model, this may not be the case on architecture like ARM and PowerPC.

Potential solutions

  1. no destructors for atomics:
    Atomics would become magic and cannot be implemented as a library
  2. Atomic wasMoved, this way atomic move can be implemented as a library
  3. type-level {.nodestroy.}:
    This would meant that the type is never destroyed implicitly
    This requires proposition 2 as well
  4. keep as-is but add a warning when an Atomic type is about to be destroyed implicitly
    This requires proposition 2 as well
mratsim added a commit to mratsim/weave that referenced this issue Nov 9, 2019
@mratsim
Copy link
Collaborator Author

mratsim commented Nov 10, 2019

Actually there is another solution.

Just like we can replace =destroy we should be able to overload wasMoved. This would be the most flexible together with warning when a type that uses atomics but no custom wasMoved is about to be destroyed.

@Araq
Copy link
Member

Araq commented Nov 10, 2019

Sounds feasible.

@Araq
Copy link
Member

Araq commented Nov 11, 2019

This is not the same as the compiler could reorder the nil statement and violates the moRelease requirements (not here due to data dependencies).

I'm not sure this is true btw, atomics have "publish" semantics, the compiler cannot reorder
non-atomic stores to come after atomic stores.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants