Revert "Optimize poll loop slightly"

[richarddd]

This reverts commit ad762253516e7230f14a9b6984b13250b811a5d9.

Fixes https://github.com/DelSkayn/rquickjs/issues/224

[DelSkayn]

Good catch! I didn't think about new futures being added while polling, however, won't the reverted code have the same problem as the current?

I believe the iterator returned by iter_mut will also be invalidated if a value is pushed into the vector, as they consist of just two pointers pointing to the memory of the vector.

The ultimate problem is that we have aliasing mutable references which will always be unsound.

I think the only proper solution is to implement the schedular with a linked list.

[richarddd]

Good catch! I didn't think about new futures being added while polling, however, won't the reverted code have the same problem as the current?

Nope, because we are not mutating the array from inside the iterator. We are simply collecting indexes.

I believe the iterator returned by iter_mut will also be invalidated if a value is pushed into the vector, as they consist of just two pointers pointing to the memory of the vector.

The ultimate problem is that we have aliasing mutable references which will always be unsound.

I think the only proper solution is to implement the schedular with a linked list.

A double linked list might be an alternative but i suspect pointer following might have slight performance implications for lots of spawns.

[DelSkayn]

Nope, because we are not mutating the array from inside the iterator. We are simply collecting indexes.

I disagree, the for loop holds onto a iterator to the futures vector which is just a pointer to the memory of the vector. Then inside the for loop poll gets called which can push a new value into the futures vec. If that happens the futures vec memory might need to be reallocated, which can cause the memory which the iterator pointer is pointing to, to become invalid resulting, again, in a segfault.

We could possibly change to using indexes in the array, but I believe, because we are supposed to have exclusive access to the futures vec the compiler could decide to still optimize the array index away to again using pointers.

A double linked list might be an alternative but i suspect pointer following might have slight performance implications for lots of spawns.

If it is done right the schedular wouldn't even have to follow pointers that often. I have been thinking about implementing the schedular with two linked lists such that whenever a future calls wake on the waker, that waker moves the future into the should be polled list from the sleeping list. That way the shedular knows which futures to poll again, avoiding the possible quadratic complexity the schedular currently has.

[richarddd]

Nope, because we are not mutating the array from inside the iterator. We are simply collecting indexes.

I disagree, the for loop holds onto a iterator to the futures vector which is just a pointer to the memory of the vector. Then inside the for loop poll gets called which can push a new value into the futures vec. If that happens the futures vec memory might need to be reallocated, which can cause the memory which the iterator pointer is pointing to, to become invalid resulting, again, in a segfault.

We could possibly change to using indexes in the array, but I believe, because we are supposed to have exclusive access to the futures vec the compiler could decide to still optimize the array index away to again using pointers.

A double linked list might be an alternative but i suspect pointer following might have slight performance implications for lots of spawns.

If it is done right the schedular wouldn't even have to follow pointers that often. I have been thinking about implementing the schedular with two linked lists such that whenever a future calls wake on the waker, that waker moves the future into the should be polled list from the sleeping list. That way the shedular knows which futures to poll again, avoiding the possible quadratic complexity the schedular currently has.

You are absolutely right, my bad. I think using two linked lists is a fantastic idea 👌

[richarddd]

Closing in favor of https://github.com/DelSkayn/rquickjs/pull/228