[kepler-dev] [Ptolemy] AbstractReceiver.java

[Daniel Crawl]

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
>>>
>