On Thu, 7 Apr 2011, Martin Stjernholm, Roxen IS @ Pike developers forum wrote:
Recursion (or rather reentrancy) detection won't do. It's not only about odd code that fiddles with call outs from operators: Since a thread switch can occur anywhere in pike code, it could cause random failures in unrelated code in other threads.
yes, you are right. I did not think of threads, only assuming that weird `== and __hash callbacks could be a problem.
There's not much that can be done against bad hashing if the same function is added a gazillion times, I guess. But otoh the large buckets don't matter except in the specific code you mentioned, right?
Yes, I think its the only case. I guess that having many call_outs to only a few different functions is not so uncommon. Doing a lot of find_call_out/remove_call_out calls might be rare.
Performance aside, it's not good that it can consume an unbounded amount of svalue stack either. At the very least there should be a check_stack() in there somewhere.
Hm, since bucket size is not known beforehand, it might be necessary to keep track of that.
I don't see any easy way of getting rid of the loop that copies the whole bucket. But something that ought to mitigate most of the problem would be to only push functions that aren't is_identical to each other, because is_eq should never behave differently for them (we can safely ignore silliness like random() in `==). I reckon simply comparing with the previous entry on the stack should be good enough.
Yes, that sounds good. So then, along your lines I would propose the following patch. It resolves the tests I have done (1000x speedup).
diff --git a/src/backend.cmod b/src/backend.cmod index e016b7b..7c2c308 100644 --- a/src/backend.cmod +++ b/src/backend.cmod @@ -966,6 +966,7 @@ PIKECLASS Backend return c->pos; } } + return -1; } /* Note: is_eq() may call Pike code (which we want), * but Pike code may change the hash tables... @@ -981,6 +982,14 @@ PIKECLASS Backend if(CALL(c->pos) != c) Pike_fatal("Call_out->pos not correct!\n"); #endif + /* We dont need to check any others. These two + * will be equal. + */ + if (is_identical(fun, ITEM(c->args))) { + pop_n_elems(Pike_sp - save_sp); + return c->pos; + } + push_int(c->pos); push_svalue(ITEM(c->args)); }