JPPF Issue Tracker
star_faded.png
Please log in to bookmark issues
enhancement_small.png
CLOSED  Enhancement JPPF-118  -  Improvements in NodeExecutionManagerImpl
Posted Jan 24, 2013 - updated Dec 27, 2014
action_vote_minus_faded.png
0
Votes
action_vote_plus_faded.png
icon_info.png This issue has been closed with status "Closed" and resolution "RESOLVED".
Issue details
  • Type of issue
    Enhancement
  • Status
     
    Closed
  • Assigned to
     lolo4j
  • Progress
       
  • Type of bug
    Not triaged
  • Likelihood
    Not triaged
  • Effect
    Not triaged
  • Posted by
     lolo4j
  • Owned by
    Not owned by anyone
  • Category
    Node
  • Resolution
    RESOLVED
  • Priority
    Normal
  • Targetted for
    icon_milestones.png JPPF 3.3.x
Issue description
This is based on a discussion with an advanced JPPF user, who pointed out a number of weaknesses and inconsistencies and possible enhancements in the way NodeExecutionManagerImpl handles the execution of the tasks within a node. Here's the list:

1) in the current version we have two maps that maintain the association between each task and the corresponding future for the node's executor:
protected final Map<Long, NodeTaskWrapper> taskMap = new Hashtable<Long, NodeTaskWrapper>();
protected final Map<Long, Future<?>> futureMap = new Hashtable<Long, Future<?>>();
The link is made by the common Long value. This is very inefficient and the 2 maps should be merged into a single one:
protected final Map<NodeTaskWrapper, Future<?>> futureMap = new IdentityHashMap<NodeTaskWrapper, Future<?>>();
This will also simplify the code maintaining the associations.

2) We use an Executor to execute the tasks but still use Object.wait() and Object.notify() to synchronize on the completion of a task bundle. This could be much simplified by using an ExecutorCompletionService on top of the executor and receiving the results asynchronously until they're all recieved.

3) The synchronization within NodeExecutionManagerImpl is too broad. Instead of using synchronized method we shoudl use more granular synchronization on specific objects on which cocurrency applies:
synchronized(futureMap) {
  // ... do some stuff ...
}

#3
Comment posted by
 lolo4j
Feb 16, 09:07
Done. All synchronization code has been removed from the execution manager.

The issue was updated with the following change(s):
  • This issue has been closed
  • The status has been updated, from New to Closed.
  • This issue's progression has been updated to 100 percent completed.
  • The resolution has been updated, from Not determined to RESOLVED.
  • Information about the user working on this issue has been changed, from lolo4j to Not being worked on.
#4
Comment posted by
 lolo4j
Feb 25, 08:32
Also implemented in branch b3.2 revision 2645
#6
Comment posted by
 lolo4j
May 21, 22:21
Reopening this ticket to track further improvements, namely changes to decouple NodeExecutionManagerImpl from NodeTaskWrapper, such that each NodeTaskWrapper holds its own future. This allows us to (finally!) get rid of the mapping of NodeTaskWrapper to corresponding future.

#11
Comment posted by
 lolo4j
May 21, 22:28
This is now implemented. Changes committed to SVN:

The issue was updated with the following change(s):
  • This issue has been closed
  • The status has been updated, from New to Closed.
  • This issue's progression has been updated to 100 percent completed.
  • The resolution has been updated, from Not determined to RESOLVED.