Clozure / ccl

Clozure Common Lisp
http://ccl.clozure.com
Apache License 2.0
857 stars 103 forks source link

Add SEMAPHORE-COUNT #308

Open phoe opened 4 years ago

phoe commented 4 years ago

If I understand correctly, the semaphore count can be accessed using a variation of the following:

? (defvar *semaphore* (make-semaphore))
*SEMAPHORE*
? (ccl::semaphore-value *semaphore*)
#<A Foreign Pointer [gcable] #x7F6410004E70>
? (ccl::%get-unsigned-long-long *)
0

Are there any multithreading gotchas that one should be aware of while doing so? I think that pointer dereferencing would be atomic in this case.

This could be the basis of the not-yet-existing function CCL:SEMAPHORE-COUNT. Having this would allow Bordeaux Threads to implement semaphore counts across multiple implementations.

Slids commented 4 years ago

Stellian doesn't want a semaphore-count function, already asked. Arguably it's not very informative, the count could change immediately after or before retrieved and and since it's a condition variable you have no great use of it for a CAS utility...

What is weird is the make-semaphore function has no count argument, instead it just supplies 0. The bt make-semaphore function when called with count just calls ccl:signal-semaphore count times. Easy fix that I have locally... Will try a push...

phoe commented 4 years ago

What is weird is the make-semaphore function has no count argument, instead it just supplies 0.

Must be old news. See the current source of BT:

https://github.com/sionescu/bordeaux-threads/blob/6a63fed849e34b3c36f6025d301e673d4b49b164/src/impl-clozure.lisp#L97

(defun make-semaphore (&key name (count 0))
  (declare (ignore name))
  (let ((semaphore (ccl:make-semaphore)))
    (dotimes (c count) (ccl:signal-semaphore semaphore))
    semaphore))

https://github.com/sionescu/bordeaux-threads/blob/aa1bf8e2f2de067790f1154c82b919b182bc6c61/src/impl-sbcl.lisp#L102

(defun make-semaphore (&key name (count 0))
  (sb-thread:make-semaphore :name name :count count))

https://github.com/sionescu/bordeaux-threads/blob/6a63fed849e34b3c36f6025d301e673d4b49b164/src/default-implementations.lisp#L273

(defdfun make-semaphore (&key name (count 0))
    "Create a semaphore with the supplied NAME and initial counter value COUNT."
  (make-%semaphore :lock (make-lock name)
                   :condition-variable (make-condition-variable :name name)
                   :counter count))
phoe commented 4 years ago

Sorry, I misread that. Yes, ccl:make-semaphore has no :count argument, as we can see here:

https://github.com/Clozure/ccl/blob/275105afd94706d95ac955178316074931822c42/level-0/l0-aprims.lisp#L194-L208

We can also see that the value is initialized to 0. This means that we can likely pass that number as the initial :count.

For signaling, we could try an atomic incf on the pointer value to increase it by a non-1 number.

Slids commented 4 years ago

That's not what you want to do. The underlining new_semaphore function takes an int for count. All that must be done is to put a value there. I have the cl locally.

On Sun, May 10, 2020, 4:04 AM phoe notifications@github.com wrote:

Sorry, I misread that. Yes, ccl:make-semaphore has no :count argument, as we can see here:

https://github.com/Clozure/ccl/blob/275105afd94706d95ac955178316074931822c42/level-0/l0-aprims.lisp#L194-L208

We can also see that the value is initialized to 0. This means that we could try an atomic incf on the memory value to increase it by a non-1 number.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Clozure/ccl/issues/308#issuecomment-626289778, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD7Y3JY3PRIYNOHLED76NWTRQZNZHANCNFSM4M4MEQEQ .

phoe commented 4 years ago

Correct - I was also thinking of passing a count argument to semaphore-signal. I've taken that discussion to #309.

Slids commented 4 years ago

I made a pull request: https://github.com/Clozure/ccl/pull/310 for the make-semaphore function.

sionescu commented 4 years ago

I changed my mind about semaphore-count. It was this article: https://lucumr.pocoo.org/2020/1/1/async-pressure/, which makes a good argument that knowing the count can be useful even if imprecise.

phoe commented 4 years ago

I've investigated a little bit further and it seems that accessing the actual semaphore value will be platform-dependent and should therefore be done in C, next to new_semaphore in https://github.com/Clozure/ccl/blob/master/lisp-kernel/thread_manager.c#L675.

sionescu commented 2 years ago

@Slids @phoe if one of you implements semaphore-count, I'd be happy to add it to Bordeaux-threads.

phoe commented 2 years ago

Extending on my previous comment a bit:

310 added an ability to set the semaphore count from the Lisp side. The real code that creates a semaphore is on the C side:

https://github.com/Clozure/ccl/blob/6c1a9458f7a5437b73ec227e989aa5b825f32fd3/lisp-kernel/thread_manager.c#L674-L694

This function is split into three implementations for three different semaphore styles (POSIX, Mach, Windows).

We'll need a new C function, int semaphore_count(void* semaphore) { ... }, which will similarly have three implementations. They can use, respectively:

On macOS, there's seemingly no way to get a semaphore count - see this StackOverflow post. CCL would need to wrap the standard macOS semaphores in order to implement semaphore count on its own.

I won't be able to easily implement or test the macOS part since I don't have a macOS machine that I can reliably test this on.

phoe commented 2 years ago

So, in other words, @sionescu - it seems to me that adding semaphore counts on macOS requires a rework of the whole semaphore CCL mechanism, and that makes it non-trivial to implement and test, and I likely won't be the one to do it. (I'm not a macOS expert though; if anyone can thoroughly debunk me on the matter, I'll be very grateful.)

svspire commented 2 years ago

It could be done fairly easily by modifying the internal CCL semaphore data structure to add a count field, and augment ccl::signal-semaphore and ccl::wait-for-semaphore to increment/decrement this field, respectively. The new version of ccl::make-semaphore would also initialize this field.

This approach wouldn't require modifying the three underlying semaphore implementations in CCL. However, it would only return reasonably accurate results if the official CCL semaphore API was always used. If for example you passed the underlying true, OS semaphore pointer to external code via FFI, the external code could increment or decrement the true count (inside the opaque Mach data structure) and the user-visible lisp count field would no longer reflect the semaphore's true value. So if you violate the API the count would potentially be even more unreliable than semaphore counts are in general.

Is this really worth doing?

phoe commented 2 years ago

Oh right - that could be done on the CL side instead of the foreign one. Welp, that simplifies the design.

If for example you passed the underlying true, OS semaphore pointer to external code via FFI

I don't expect that to happen very often in practice - you'd need to have strictly CCL-only code that has three separate branches for three OS families and then passes the underlying raw semaphore to each one of them. Am I wrong on this matter? Has anyone encountered creating semaphore objects on the CL side and then accessing and passing the raw OS-semaphores around?

svspire commented 2 years ago

Agree that it seems very unlikely. I'm just thinking worst-case so users would be aware of the limitations.