Skip to content

Conversation

jdantonio
Copy link
Member

@mighe I've updated Concurrent::timer based on your suggestion. I think this implementation is much better. Please feel free to update if you think it can be optimized more. Here's what I did:

  • Implemented a pure Ruby, not thread safe, MutexPriorityQueue class based on Sedgewick and Wayne
  • Implemented a JavaPriorityQueue wrapper for java.util.PriorityQueue
  • Created a PriorityQueue class that maps to either the Ruby or Java implementation as appropriate
  • Created a new TimerSet executor that follows your suggested algorithm (puts all tasks in a priority queue based on absolute time then posts tasks to a thread pool)
  • Implemented a few optimizations in TimerSet such as lazy starting of the monitor thread, letting the monitor thread die when not being used, and support for the :executor option on construction
  • Mapped the global task pool behind Concurrent::timer to a TimerSet

jdantonio added a commit that referenced this pull request Apr 11, 2014
Implemented TimerSet executor and updated Concurrent::timer
@jdantonio jdantonio merged commit b421874 into master Apr 11, 2014
@mighe
Copy link
Contributor

mighe commented Apr 12, 2014

Great job!
I love the heap based queue implementation (Sedgewick's book is one of the most valuable ones I own 😊 )

I'm only a bit concerned about the TimerSet implementation: currently, with very unlucky timings, we can have unexpected results (line 150 of timer_set.rb)

        if @thread && @thread.status == 'sleep'
          @thread.wakeup
        elsif @thread.nil? || ! @thread.alive?
          @thread = Thread.new do
            Thread.current.abort_on_exception = true
            process_tasks
          end
        end

Here we're checking-and-acting over the thread status in non-atomic way (because it isn't possible): with unlucky timings we could see a sleeping thread not being woken up or not being created again when dies.

Do you feel this optimization is really valuable? The simplest way to provide a safe implementation is creating one thread the first time and keep it running forever (like Java does for Timer)... at the moment I cannot find a simple solution to solve those issues 😕

@jdantonio
Copy link
Member Author

I have a (possibly unwarranted) fear of threads that run forever. Years ago when I programmed Visual C++ we had a problem where Windows would regularly reclaim threads that were idle (sleeping) for too long. I could never find any documentation describing this issue but I was able to reproduce it in both the lab and (sadly) in production. Back then I was working on backing systems that were never shut down. So certain threads could be idle for several days if they had no work to do. Because of that I've become very defensive about threads that remain idle for a long time. It may not be a problem in modern systems, but I don't have nearly as much faith in MRI/CRuby as I do in Java.

So I think it's a question of which edge case we're more scare of: the runtime killing the thread for inactivity or performing a non-atomic check against the thread. And I honestly don't know the answer to that.

This may be a case for "if JRuby else..." logic. Maybe substitute a SingleThreadExecutor for the raw thread.

@mighe
Copy link
Contributor

mighe commented Apr 12, 2014

Please, take a look at bd7507b
I've changed a bit your implementation keeping the optimization and now I think (and hope) it is thread safe.

When the monitor thread should be killed, it atomically sets the instance thread variable to nil.
When another thread puts something in the queue, spawns a new monitor thread atomically if it is nil and sets the variable to the new thread.

All tests are still passing.
If the code seems right also to you, I'll refactor it a bit and merge into the master branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants