[kepler-dev] [Ptolemy] AbstractReceiver.java
Sean Riddle
swriddle at gmail.com
Wed May 13 10:12:27 PDT 2009
I agree that the correct approach would be to modify ArrayToSequence.
I can't remember quite why, but I also needed this behavior from
ArrayToSequence at some point, and the change is only one or two
lines, plus you're guaranteed that this won't affect other parts of
the system unexpectedly.
- Sean
Sent from my iPhone
On May 13, 2009, at 8:32 AM, "Edward A. Lee" <eal at eecs.berkeley.edu>
wrote:
>
> I see.
>
> It seems to me that the "right" fix is to modify ArrayToSequence,
> not to modify the mechanism in AbstractReceiver. The reason for
> the mechanism in AbstractReceiver is to support bulk transfers
> efficiently. I'm not sure where else this is used, but it does
> seem that preserving this capability would be useful.
>
> I don't feel that strongly about it though...
>
> Edward
>
>
> Jianwu Wang wrote:
>> Hi Edward,
>> Dan and I found the problem from a real world workflow. The
>> workflow includes about 50 job files, their execution time varies
>> from 1 hours to 5 hours. The jobs can be submitted by firing
>> JobSubmitter actor 50 times with corresponding inputs, and their
>> status can be checked by JobStatus actor in a loop. I hope once one
>> job is finished, the following actors related to the data of this
>> job can be processed without waiting for other jobs. So that we can
>> enable pipeline parallel. The workflow has the structure similar
>> with the attached test case. With default PN director
>> configuration, ArrayToSequence actor tries to send all array data
>> to one next actor before sending data to another connected actor.
>> But the Equals actor need the data from the two Expressions to be
>> fired. So the Equals actor will increase its capacity and not be
>> fired since there is still running actor (job actors). The workflow
>> has to wait all jobs are finished before processing the following
>> actors, which is very inefficient for my case.
>> With Dan's modification, the ArrayToSequence actor can transfer
>> the first token of the array to all the following actors before
>> transfer the second token. Then the Equals actor can be fired
>> timely and do not need to increase capacity. So the same workflow
>> can realize real pipeline parallel, namely the following actors can
>> be fired immediately with corresponding data once one job is
>> finished.
>> Best wishes
>> Sincerely yours
>> Jianwu Wang
>> jianwu at sdsc.edu
>> http://users.sdsc.edu/~jianwu/
>> Scientific Workflow Automation Technologies (SWAT) Laboratory
>> San Diego Supercomputer Center University of California, San Diego
>> San Diego, CA, U.S.A.
>> Edward A. Lee wrote:
>>>
>>> I don't really understand the problem being solved here.
>>> The deadlock is a consequence of setting the maximum queue capacity
>>> to one. Why would you want to do that?
>>>
>>> Edward
>>>
>>>
>>> Daniel Crawl wrote:
>>>>
>>>> Hi Edward,
>>>>
>>>> Attached is the test case. I set the max queue capacity to one so
>>>> that an exception is thrown instead of an artificial deadlock
>>>> occurring. With my change, the exception does not occur and the
>>>> model finishes.
>>>>
>>>> Is there a test case demonstrating the performance problem? In
>>>> both versions, put is called (with a single token) the same number
>>>> of times, so it's not clear how my change could hurt efficiency.
>>>>
>>>> Thanks,
>>>>
>>>> --dan
>>>>
>>>>
>>>> Edward A. Lee wrote:
>>>>>
>>>>> Dan,
>>>>>
>>>>> Are you sure the deadlock is artificial?
>>>>> I would like to see the test case. Maybe the model isn't using
>>>>> the right actors?
>>>>>
>>>>> The point of the methods you changed was to improve
>>>>> efficiency, and sending tokens one at a time nullifies
>>>>> that point. There is really not point in even having these
>>>>> methods, I think.
>>>>>
>>>>> Edward
>>>>>
>>>>>
>>>>> Daniel Crawl wrote:
>>>>>>
>>>>>> Hi Christopher,
>>>>>>
>>>>>> I made this update to prevent unnecessary artificial deadlocks
>>>>>> in PN under certain circumstances. I can add a test case that
>>>>>> demonstrates the problem.
>>>>>>
>>>>>> If the convert is performed, is the update ok? Note that no
>>>>>> tests failed in ptolemy/actor/test/ due to this change...
>>>>>> Since calling convert is essential, I can also add a test case
>>>>>> for this.
>>>>>>
>>>>>> There were effectively two nested loops before, so I do not see
>>>>>> how this change could degrade performance. If it is measurably
>>>>>> different, it is improved since the outer loop no longer calls
>>>>>> a method.
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> --dan
>>>>>>
>>>>>>
>>>>>>
>>>>>> Christopher Brooks wrote:
>>>>>>> Yep, I went ahead and reverted the change.
>>>>>>>
>>>>>>> _Christopher
>>>>>>>
>>>>>>> Edward A. Lee wrote:
>>>>>>>>
>>>>>>>> The call to convert is essential.
>>>>>>>>
>>>>>>>> Without it, we'll get some very esoteric and difficult to track
>>>>>>>> type system bugs. A likely manifestation is that actors will
>>>>>>>> start throwing ClassCastException because they have declared
>>>>>>>> an input to be double, so they cast incoming tokens to
>>>>>>>> DoubleToken.
>>>>>>>> Without the call to convert(), they may get, say, an IntToken.
>>>>>>>> This will be a very unfriendly error...
>>>>>>>>
>>>>>>>> Edward
>>>>>>>>
>>>>>>>>
>>>>>>>> Christopher Brooks wrote:
>>>>>>>>> Hi Daniel,
>>>>>>>>> I'm concerned that this is a performance hit because we
>>>>>>>>> have two nested loops. Can you tell me more about why this
>>>>>>>>> change is necessary? Do you have a test case that illustrates
>>>>>>>>> the bug? Without a test case, it is not likely that the fix
>>>>>>>>> will
>>>>>>>>> persist, though the comment should help.
>>>>>>>>>
>>>>>>>>> The entire method is:
>>>>>>>>> /** Put a sequence of tokens to all receivers in the
>>>>>>>>> specified array.
>>>>>>>>> * Implementers will assume that all such receivers
>>>>>>>>> * are of the same class.
>>>>>>>>> * @param tokens The sequence of token to put.
>>>>>>>>> * @param numberOfTokens The number of tokens to put
>>>>>>>>> (the array might
>>>>>>>>> * be longer).
>>>>>>>>> * @param receivers The receivers.
>>>>>>>>> * @exception NoRoomException If there is no room for
>>>>>>>>> the token.
>>>>>>>>> * @exception IllegalActionException If the token is not
>>>>>>>>> acceptable
>>>>>>>>> * to one of the ports (e.g., wrong type), or if the
>>>>>>>>> tokens array
>>>>>>>>> * does not have at least the specified number of tokens.
>>>>>>>>> */
>>>>>>>>> public void putArrayToAll(Token[] tokens, int
>>>>>>>>> numberOfTokens,
>>>>>>>>> Receiver[] receivers) throws NoRoomException,
>>>>>>>>> IllegalActionException {
>>>>>>>>> if (numberOfTokens > tokens.length) {
>>>>>>>>> IOPort container = getContainer();
>>>>>>>>> throw new IllegalActionException(container,
>>>>>>>>> "Not enough tokens supplied.");
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> // Put a single token at a time for each receiver
>>>>>>>>> instead of
>>>>>>>>> // putting the entire array. In the latter case, we
>>>>>>>>> may block
>>>>>>>>> // on a receiver while other receiver(s) starve.
>>>>>>>>> for(int i = 0; i < numberOfTokens; i++) {
>>>>>>>>> for (int j = 0; j < receivers.length; j++ ) {
>>>>>>>>> receivers[j].put(tokens[i]);
>>>>>>>>> }
>>>>>>>>> }
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I do see how this could be a problem with blocking though.
>>>>>>>>>
>>>>>>>>> Your change is to call put() instead of putArray().
>>>>>>>>> AbstractReceiver.putArray() looks like:
>>>>>>>>>
>>>>>>>>> public void putArray(Token[] tokenArray, int
>>>>>>>>> numberOfTokens)
>>>>>>>>> throws NoRoomException, IllegalActionException {
>>>>>>>>> IOPort container = getContainer();
>>>>>>>>>
>>>>>>>>> // If there is no container, then perform no
>>>>>>>>> conversion.
>>>>>>>>> if (container == null) {
>>>>>>>>> for (int i = 0; i < numberOfTokens; i++) {
>>>>>>>>> put(tokenArray[i]);
>>>>>>>>> }
>>>>>>>>> } else {
>>>>>>>>> for (int i = 0; i < numberOfTokens; i++) {
>>>>>>>>> put(container.convert(tokenArray[i]));
>>>>>>>>> }
>>>>>>>>> }
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> It looks like your change is ok when the container is null,
>>>>>>>>> but
>>>>>>>>> in the AbstractReceiver base class it does not handle the call
>>>>>>>>> to convert()? I'm not sure if this is important or not.
>>>>>>>>>
>>>>>>>>> I'm fairly certain that putArrayToAll() will be called when
>>>>>>>>> we call
>>>>>>>>> IOPort.broadcast.
>>>>>>>>>
>>>>>>>>> What do you think?
>>>>>>>>>
>>>>>>>>> _Christopher
>>>>>>>>>
>>>>>>>>> Daniel Crawl wrote:
>>>>>>>>>> Author: crawl
>>>>>>>>>> Date: 2009-05-06 14:45:46 -0700 (Wed, 06 May 2009)
>>>>>>>>>> New Revision: 53516
>>>>>>>>>>
>>>>>>>>>> Modified:
>>>>>>>>>> trunk/ptolemy/actor/AbstractReceiver.java
>>>>>>>>>> Log:
>>>>>>>>>> Put a single token at a time for each receiver in
>>>>>>>>>> putArrayToAll().
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Modified: trunk/ptolemy/actor/AbstractReceiver.java
>>>>>>>>>> ===
>>>>>>>>>> ===
>>>>>>>>>> =============================================================
>>>>>>>>>> --- trunk/ptolemy/actor/AbstractReceiver.java 2009-05-06
>>>>>>>>>> 21:13:26 UTC (rev 53515)
>>>>>>>>>> +++ trunk/ptolemy/actor/AbstractReceiver.java 2009-05-06
>>>>>>>>>> 21:45:46 UTC (rev 53516)
>>>>>>>>>> @@ -300,8 +300,13 @@
>>>>>>>>>> "Not enough tokens supplied.");
>>>>>>>>>> }
>>>>>>>>>> - for (int j = 0; j < receivers.length; j++) {
>>>>>>>>>> - receivers[j].putArray(tokens, numberOfTokens);
>>>>>>>>>> + // Put a single token at a time for each receiver
>>>>>>>>>> instead of
>>>>>>>>>> + // putting the entire array. In the latter case,
>>>>>>>>>> we may block
>>>>>>>>>> + // on a receiver while other receiver(s) starve.
>>>>>>>>>> + for(int i = 0; i < numberOfTokens; i++) {
>>>>>>>>>> + for (int j = 0; j < receivers.length; j++ ) {
>>>>>>>>>> + receivers[j].put(tokens[i]);
>>>>>>>>>> + }
>>>>>>>>>> }
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> _______________________________________________
>>>>>>>>>> Ptexternal-cvs mailing list
>>>>>>>>>> Ptexternal-cvs at chess.eecs.berkeley.edu
>>>>>>>>>> http://chess.eecs.berkeley.edu/ptexternal/listinfo/ptexternal-cvs
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> Ptolemy maillist - Ptolemy at chess.eecs.berkeley.edu
>>>>>> http://chess.eecs.berkeley.edu/ptolemy/listinfo/ptolemy
>>>>
>>> _______________________________________________
>>> Kepler-dev mailing list
>>> Kepler-dev at kepler-project.org
>>> http://mercury.nceas.ucsb.edu/kepler/mailman/listinfo/kepler-dev
>>>
>> _______________________________________________
>> Ptolemy maillist - Ptolemy at chess.eecs.berkeley.edu
>> http://chess.eecs.berkeley.edu/ptolemy/listinfo/ptolemy
> <eal.vcf>
> _______________________________________________
> Kepler-dev mailing list
> Kepler-dev at kepler-project.org
> http://mercury.nceas.ucsb.edu/kepler/mailman/listinfo/kepler-dev
More information about the Kepler-dev
mailing list