Allow fibers to have a wider range of priorities

[voidstarstar]

As discussed in #195.

This PR can be used if having a wider range of priorities turns out to be useful. Fibers are no longer restricted to the same priority range as a Thread. Hopefully this does not complicate the code too much.

I'm not sure if this follows correct code conventions consistent with the rest of the project, so please let me know if there are any minor things that I should fix.

@pron I realize that you already stated that you are reluctant to make this change unless there is a compelling reason, so please treat this PR as merely a suggestion in the event that a compelling reason arises. If there is not a good reason to make the change, then feel free to reject the PR. I just wanted to post this in case it is useful to anyone or saves you some time. :smile:

[voidstarstar]

It looks like the test failed due to integer overflow and underflow. This seems like a bug in the test, so I also pushed a change to exclude these two special cases from failing.

[pron]

I am not going to merge it at this time, but in any case, there's a mistake in your code: MIN_PRIORITY and MAX_PRIORITY cannot be static methods. The strand creating the prioritized strand and assigning it a priority may not be of the same kind. A thread could create a fiber and vice-versa.

[voidstarstar]

Thanks for the feedback!

I just fixed one small bug, but I can't find the bug that you're describing.

The static methods are never actually used when creating a Fiber or Thread. The Fiber constructor and setPriority() method now use static fields directly in Fiber rather than in Strand to better match what the Thread class does. I don't think that the MIN_PRIORITY() and MAX_PRIORITY() methods are actually used anywhere or are really necessary, so they can probably be removed. I'll push another commit to remove these static methods completely.

I probably caused some confusion by naming them the same as the removed static fields and not updating the setPriority() Javadoc to reflect the change.

Let me know if I'm misunderstanding what you mean.

[disposableme]

Thank you both for your work on this issue!

I think that having only 10 priorities is too limiting for my use case. Having a range from -2³¹ to 2³¹-1 is better, but still not general. I think that ideally either a Comparator should be used or have an Actor implement Comparable and this would propagate somehow down to the Fiber.

As discussed previously, there may be some contention on the queue, but I think it would be nice to leave it up to the user to decide if the trade-off is worth it.

[pron]

@disposableme

1. I think a Phaser would solve your issue likely better than priorities.
2. The new changes allow you to use relative ordering, as the scheduler now has access to the fiber. Just subclass the fiber and implement Comparable.

[voidstarstar]

@disposableme

I've actually been working on a proof of concept for using relative ordering. You can find the repository here.

I've managed to get it working except it would require 2 small changes to the Actor class. (I'd be happy to create a PR if you'd like.)

1. The runner field would need to be protected instead of private.
2. The checkReplacement method would need to be protected instead of private.

Both of these class members could probably also be made final as well.

This is necessary because I am making a subclass of Actor in order to override the spawn(FiberFactory) method. I'm creating 3 subclasses in total:

1. BasicActor
2. Fiber
3. FiberExecutorScheduler

I may be overlooking something, so please let me know if you know of a simpler solution.

[pron]

@disposableme

I don't understand. Why not just subclass Fiber, and pass the actor a FiberFactory that returns those Comparable fibers?

You could use it like so

myActor.spawn(new PrioritizedFactory(someComparator))

Actors should not be at all involved with scheduling.

[voidstarstar]

Actors should not be at all involved with scheduling.

Thanks for the advice. I've updated the example repo to no longer require a subclass of BasicActor and it therefore also no longer requires those 2 visibility changes to Actor.

This update, however, now requires a subclass of RunnableFiberTask and a visibility change to the newFiberTask method in FiberScheduler.

If the visibility of newFiberTask is changed, then the Fiber priorities would probably not be required at all and this PR could be rejected/closed.

A somewhat unimportant side note: The RunnableFiberTask.fiber field is also private. In my RunnableFiberTask subclass, I had to create another field to hold the same fiber reference, so it may be convenient, but not necessary, to change the visibility of this field. The RunnableFiberTask subclass requires this reference to the entire Fiber rather than just the Comparable field because the Fiber constructor calls the newFiberTask method before the comparable field can be initialized in the Fiber subclass. I believe that this is due to this suppressed warning.

[pron]

I don't think you need to subclass RunnableFiberTask, as you now have access to the fiber itself and are free to subclass it as you like.

[voidstarstar]

@pron Thanks for the feedback! Hopefully I'm now implementing this better.

You were right, creating that subclass was unnecessary. Prioritizing the tasks is possible without any modification to the quasar code. :smile:

I think I was trying to subclass RunnableFiberTask to make it implement Comparable, but as you said, this is unnecessary now that FiberFactory is public.

I pushed some changes to remove the need to subclass of RunnableFiberTask. Also, one interesting note is if the Comparable is mutable, then the actor can also have dynamic ordering (change its priority on-the-fly), such as in response to a received message. I created another branch with a simple example to demonstrate this capability. (Note: This example uses Thread.sleep() in order to simulate longer running code that will lead to an accumulation of sorted tasks in the queue. This should obviously not be used in production.)

Thanks again for your help @pron!

@disposableme I hope this code helps!