[jira] [Commented] (ETCH-258) Switch to using util.concurrent instead of pre Java 5 threading constructs

["Paul Turner (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/ETCH-258?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13591537#comment-13591537 ] 

Paul Turner commented on ETCH-258:
----------------------------------

Summary
I have not changed things just to change them.  The changes I have made to convert to using util.concurrent classes are mainly for reasons such as ability to recycle threads.

----------
org.apache.etch.util.TodoManager
Motivation for change:
TodoManager is a threadpool with a capped queue capacity.  Methods to schedule the execution of a Todo are synchronized,.
System performance will  degrade as too many requests are being created for  the threadpool  and when it cannot service them.  This means that method calls will accumulate whilst waiting for the instance scoped lock and will use memory beyond the capacity of the work queue.  In effect, waiting synchronized method calls is a hidden and unbounded queue.

Change: 
An approach used in the java.util.concurrent threadpool framework to deal with this degradation is to force calling threads to carry out the execution of their own items scheduled on the threadpool.  
This is achieved by using a CallerRunsPolicy as the RejectedExecutionHandler in ThreadPoolExecutor.
This has 2 beneficial effects:
1: The producer threads ability to saturate the threadpool is limited because more more of their time is spent servicing requests rather than producing them.
2: The amount of work to be done, saturation of threadpool and memory used in calls awaiting instance lock is reduced.

I have changed the TodoManager class to use standard ExecutorService framework for the reasons stated above.

Consequence of Change:
A feature of the original TodoManager is lost.  This is:
Ability to sleep while waiting for work queue capacity.  (As discussed previously)
This was configured by the entryDelay in the constructor of TodoManager.  The old constructor remains for backwards-compatibility but is @Deprecated.
All public methods remain with the same contract.
Unit test added.

----------
org.apache.etch.util.AlarmManager
Motivation for change:
Adding alarms does not take advanatage of Java standard api classes.

Change:
I have changed the AlarmManager class to use a ScheduledExecutor.  All public methods remain with the same contract.

----------
org.apache.etch.util.IdGenerator
Motivation for change:
Use standard Java apis for atomic incrementing number functionality

Change:
Instance level synchronization no longer necessary, An AtomicLong now takes care of atomic incrementor functionality.  All public methods remain with the same contract.  Unit test added.

----------
org.apache.etch.bindings.java.support.FreePool
Motivation for change:
This is a threadpool and would benefit from the use of standard Java api classes.
Change:
Threadpool functionality now provided by Executors framework.  A SynchrouousQueue is used to ensure a queue-free implementation rejecting runnables with an exception where necessary.
Overall effect is that threads are recycled by the threadpoolexecutor rather than being re-created for each task.  All public methods remain with the same contract.

----------
org.apache.etch.bindings.java.support.TestFreePool
TestFreePool.java
Motivation for change:
Unit tests were unpredictably failing due to a race condition in that the asynchronous PoolRunnable execution may not have completed before the unit test Asserts are called.

Change:
The main thread of the unit test is now blocked by a CountDownLatch until all the asychronous PoolRunnables complete.  I also added another test to make sure the SynchronousQueue used as the threadpool BlockingQueue calles the RejectedExecutionHandler as expected. (Basically a test for throwing an Exception when pool full -> probably a repeat of existing tests).

----------
Remaining Work
I would like to make the Monitor support concurrent waitUntil* calls.  This is because the current implementation blocks on e.g. waitUntilEq and it is not possible to lock acquire in a fair way using the synchronized keyword.  Out of 2 synchronized calls to waitUntilEq one will obtain the lock and the other will wait.  After the first method call has released the lock a call to set() could be executed before the second call to waitUntilEq(...) obtains the lock.  This means that  when the second call to waitUntilEq eventually acquires the lock it could see a different value that was present when the method was invoked.  This is very difficult because coordination between waitUntil*(...) and set(...) require the use of a lock but once this lock has been acquired and the blocking call made to wait for changes, it is not possible to release the lock to enable other calls to set(..) or waitUntil*(...).

A test case for this is as follows.

A monitor is set up with value = 1 and then to calls are made in separate threads to waitUntilEq.  One waiting for 2, and later another waiting for 1.
Expected result: The call to waitUntilEq(2) will block indefinitely,  the call to waitUntilEq(1) should return immediately.
Observed result: Both calls to waitUntilEq(...) block indefinitely.
I have added this test case to TestMonitor but without the @Test annotation so as not to affect the build.

I would also like to refactor the CircularQueue to use a BlockingDeque as the internal double-ended queue.
                
> Switch to using util.concurrent instead of pre Java 5 threading constructs
> --------------------------------------------------------------------------
>
>                 Key: ETCH-258
>                 URL: https://issues.apache.org/jira/browse/ETCH-258
>             Project: Etch
>          Issue Type: Improvement
>          Components: binding-java, general
>            Reporter: Paul Turner
>            Priority: Minor
>             Fix For: 1.4
>
>         Attachments: etch-20130301.patch
>
>   Original Estimate: 168h
>  Remaining Estimate: 168h
>
> thread creation is quite expensive and so a new thread per unit of work is also expensive, i propose to use util.concurrent threadpools in the java binding sub-project and enhance unit tests e.g. with countdown latches to ensure competing test threads start simeltanously and semaphore to throttle access to running units of work.
> affects FreePool, TodoManager and associated tests and possibly more classes

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira