[kepler-dev] [Ptolemy] AbstractReceiver.java
Edward A. Lee
eal at eecs.berkeley.edu
Tue May 12 22:40:21 PDT 2009
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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: eal.vcf
Type: text/x-vcard
Size: 364 bytes
Desc: not available
URL: <http://mercury.nceas.ucsb.edu/kepler/pipermail/kepler-dev/attachments/20090512/5d660c57/attachment.vcf>
More information about the Kepler-dev
mailing list