Yes, I didn't touch them yet. It would be good to have someone look at it in get(), on_success() and on_failure(), since they will get a lot of calls.

In fact, why is there a lock in on_success and on_failure at all?

...

In fact, why is there a lock in on_success and on_failure at all?

To make sure that callbacks are called exactly once per failure/success.

Can you explain that better? The only issue I can see is 1) it failes the state check 2) a different thread changes the state and consumes the array 3) cb is added to the array and never executed. Now, this can't happen as there is no function call between the state variable check and the array append. Even if it could happen, a mutex in on_success/on_failure wouldn't help, as it is access to the state variable that needs to be guarded.

Lots of code depends on the interpreter lock. With something as core as Concurrent we really need both correctness and performance. First step would be to write good performance benchmarks though.

Stephen R. van den Berg wrote:

Martin Nilsson (Coppermist) @ Pike (-) developers forum wrote:

...

Lots of code depends on the interpreter lock. With something as core as Concurrent we really need both correctness and performance. First step would be to write good performance benchmarks though.

Ok. I'll run through it once more and optimise.

I ran some tests on Pike 8.0 and 8.1 using the following program:

--------------------cut here----------------- Thread.Mutex mux = Thread.Mutex(); int c = 1;

class des { string label; void create(string lab) { label = lab; werror("Created! %s ******************\n", label); } void destroy() { werror("Destroyed! %s ***************\n", label); } }

void a(int ... v) { werror("a...\n"); des y = des("yy"); { des x = des("xx"); Thread.MutexKey lock = mux->lock(); werror("a got lock\n"); sleep(0.5); c++; //sleep(0.000001); } //sleep(0.000001); c++; werror("a out of sleep\n"); c++; sleep(0.5); c++; werror("a unlocked\n"); c++; }

void b(int ... v) { werror("b...\n"); Thread.MutexKey lock = mux->lock(); werror("b got lock %d\n", c); sleep(1); werror("b out of sleep\n"); lock = 0; werror("b unlocked\n"); }

int main(int argc, array(string) argv) { Thread.Thread(a); sleep(0.2); Thread.Thread(b); sleep(2); return 0; } --------------------cut here-----------------

When running it (repeatedly), I notice the following things: a. To my delight I see that object destructors are consistently being run at the closing brace of a block. Cheers! (Makes me wonder when this changed, because in 7.8 it did not work this way yet, I think). b. The moment when it switches to thread b varies, depending on chance/timing. c. Sometimes at switch time c is either 2, or 3, or 4. Which is fine. d. Note that despite one would expect 2 to be impossible due to the interpreter lock, it is possible because running the destructors at the end of that block probably breaks that promise.

So it appears that I can reliably get rid of some of the MutexKey = 0 assignments. And, I'll start relying on the interpreter lock for the trivial cases (pointer and immediate use of that pointer). In addition, I'll scrutinise the hotspot places of mutexes to see if we can get by without a mutex if the interpreter lock can trivially be utilised.

Martin Nilsson (Coppermist) @ Pike (-) developers forum wrote:

Why do we delay object destruction btw?

It appears that that is not the case anymore.

If I recall correctly, mast once explained to me that destructors of objects on the stack are called some time after a function has ended. I think I actually checked that, and that must have been in Pike 7.8.

The testprogram I just listed in the previous mail, seems to indicate that all objects are now destructed as soon as the scope closes. I.e. the delayed destruct is not happening any longer, regardless of the IMMEDIATE_DESTRUCT flag. It seems to work reliably for any nested block; which is great, because it makes it very intuitive to guess when destructors are being executed.

Martin Nilsson (Coppermist) @ Pike (-) developers forum wrote:

And speaking of Concurrent, since MutexKey is destructed immediately when it goes out of scope, I think many of the key=0 can be removed from the code.

Is that new in 8.0 or 8.1? In 7.8, it could take an undetermined amount of time before it would be released if not assigned to zero explicitly before the end of the function.

I have very bad experiences with MutexKeys being held for way too long and causing deadlocks (and I have a *lot* of experience on deadlocks in the pgsql driver).

Martin Nilsson (Coppermist) @ Pike (-) developers forum wrote:

...

I have very bad experiences with MutexKeys being held for way too long and causing deadlocks (and I have a *lot* of experience on deadlocks in the pgsql driver).

Immediate destruct was added 1998. That doesn't mean it is bug free, but for such trivial use case as here I would be surprised if it was not, since it is used a lot in Roxen Webserver.

Well, maybe that is true at the end of a function in current Pike. Nonetheless: a. It does not hold true for other blocks besides functions. b. It is sensitive to tail call optimisation. c. Assigning 0 explicitly to MutexKeys everywhere makes it very explicit when it unlocks and makes it immune to code relocation. d. When I assign 0 to such a variable, and the Pike compiler sees that, is it then not possible for the optimiser to recognise that the assignment is redundant because we are about to leave functionscope anyway and then optimise the assignment away?

In the same veign, can the Pike optimiser not recognise this:

if (err) { Promise p = promise; // Cache it, to cover a failure race if (p) p->failure(err); }

And then just fetch the value of promise once, and then use it twice without us having to explicitly convert the code into:

if (err && promise) promise->failure(err);

?