nim-works / loony

A high throughput MPMC lock-free queue based on a paper by Giersch, Nolte et al implemented in pure Nim.
https://nim-works.github.io/loony/
MIT License
64 stars 4 forks source link

problem with strings, something I should know about? #28

Closed SirStone closed 1 year ago

SirStone commented 1 year ago

Hi, I'm quite new to Nim and I work on it occasionally during weekends.

I'm testing loony for a simple double thread producer/consumer paradigm. Here's the code:

import std/[locks, os]
import loony

type
  Message = object
    value: string

let fifo:LoonyQueue[Message] = newLoonyQueue[Message]()
var L: Lock
var producing = true

proc producer() {.thread.} =
  for i in 1..10:
    let Message = Message(value: "Message " & $i)
    acquire(L)
    fifo.push Message
    release(L)
    echo "Produced: ", Message
    sleep(100)

proc consumer() {.thread.} =
  while producing:
    acquire(L)
    let item = fifo.pop
    if item.value != "":
      echo "Consumed: ", item
    release(L)
    sleep(10)

# Create worker threads
var producerThread, consumerThread: Thread[void]

initLock(L)

# Start worker threads
createThread(producerThread, producer)
createThread(consumerThread, consumer)

joinThread(producerThread)
producing = false
joinThread(consumerThread)

deinitLock(L)

It's a simple, documentation-alike, piece of software, but when I run it I get this output:

Produced: (value: "Message 1")
Produced: (value: "Message 2")
Produced: (value: "Message 3")
Consumed: (value: "��\x0Ew\x14�\x03@")
Produced: (value: "Message 4")
Produced: (value: "Message 5")
Consumed: (value: "��\x0Ew\x14�\x03@")
Produced: (value: "Message 6")
Produced: (value: "Message 7")
Consumed: (value: "��\x0Ew\x14�\x03@")
Produced: (value: "Message 8")
Produced: (value: "Message 9")
Consumed: (value: "��\x0Ew\x14�\x03@")
Produced: (value: "Message 10")
Consumed: (value: "��\x0Ew\x14�\x03@")

I would like to talk about the weird codes I get from the popped Messages' values and asking if there's something I don't know about strings that explain this. I also tested the same code using a LoonyQueue[string] directly, and in this case the code didn't compile at all

Using MacOS Catalina 10.15.7, nim 1.6.12, latest loony

shayanhabibi commented 1 year ago

I'm away until tomorrow and have not touched programming with a 5-foot pole in almost two years. @disruptek hopefully isn't as grouchy as I remember and can help.

Otherwise I think the principle error is that your Message is a Object and not a ref object. Therefore, the message it contains is instantaneously destroyed when you put it into the loony. Loony has nothing to do with memory allocation/deallocation outside of its own function. HOWEVER, we will do the appropriate memory protective functions for a native 'ref' object (within the means provided by std lib which is not atomic back when we made this).

Also no locks. Why are you using locks for a lockfree algorithm? You're essentially getting an F1 race car and then towing it with a truck to work and wondering why it takes you the same amount of time to get to work.

Please try compile with Message as a ref object.

type
  Message = ref object
    value: string

note: this assumes you are using ARC or ORC memory management paradigms.

Let me know how you go.

shayanhabibi commented 1 year ago

I just realised you are initialising your message objects in a profoundly error-prone and obscure way.

Rather than this: let Message = Message()

Do this: let message = Message()

shayanhabibi commented 1 year ago

I just realised there are fundamentally a few things wrong with your example code that I cannot provide a satisfactory correction for on my iPad. I'll provide you a corrected sample of code tomorrow (although my memory is notoriously bad and I might forget - don't hesitate to remind me).

disruptek commented 1 year ago
import std/[locks, os, atomics]
import loony

type
  Message = ref object
    value: string

let fifo = newLoonyQueue[Message]()
var terminate: Atomic[bool]

proc producer() {.thread.} =
  for i in 1..10:
    let msg = Message(value: "Message " & $i)
    echo "Producing ", repr(msg)
    fifo.push msg
    sleep(100)

proc consumer() {.thread.} =
  while not terminate.load:
    let item = fifo.pop
    if not item.isNil:
      echo "Consumed: ", repr(item)
    sleep(10)

# Create worker threads
var producerThread, consumerThread: Thread[void]

# Start worker threads
createThread(producerThread, producer)
createThread(consumerThread, consumer)

joinThread(producerThread)
terminate.store(true)
joinThread(consumerThread)
SirStone commented 1 year ago

Thanks for the fast reply, I will learn more from this code of your than hours spent figuring out myself.

ok let me say that the let Message = Message() was a mistake cause by Copilot that I missed cause I was coding under the sun, visibility 20% my bad. The rest of the suggestions...it's gold for me, I'm not aware of what Atomic is and I was not aware for the differences between = object and = ref object. Still my bad and ignorance is not an exscuse. The locks, I've added those out of madness, let's just pretend it didn't happen >_>

I've already compiled and run your code, now I'm going to study it.

I want ask another basic question, what is the best method to check that the queue is not empty? I see that there's the isEmpty call but the documentations recites This operation should only be used by internal code. The response for this operation is not precise. suggesting to not use it.

I'm going out of topic, this issue can be closed

shayanhabibi commented 1 year ago

Thanks @disruptek, much appreciated for that. @SirStone you have no reason to apologise. Everyone starts from somewhere and the nuances of nim can only be found through trial and error or by looking under the hood. I'll be happy to provide answers for all of the above in a discussion or some other format.

shayanhabibi commented 1 year ago

I still firmly believe the following resource is invaluable for any acolyte nimmers: https://github.com/StefanSalewski/NimProgrammingBook

Dr Stefan Salewski has created a fantastic resource which covers all the nuances of nim fantastically.