[kepler-dev] [Ptolemy] AbstractReceiver.java
Christopher Brooks
cxh at eecs.berkeley.edu
Mon May 11 13:05:27 PDT 2009
Hi Daniel,
I folded a change similar to yours:
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.");
}
// Loop through the tokens on the outer loop and
// the receivers on the inner loop. See
// pn/kernel/test/block.xml for a test case
// (Bug fix proposed by Daniel Crawl.)
for(int i = 0; i < numberOfTokens; i++) {
for (int j = 0; j < receivers.length; j++ ) {
IOPort container = receivers[j].getContainer();
if (container == null) {
receivers[j].put(tokens[i]);
} else {
receivers[j].put(container.convert(tokens[i]));
}
}
}
}
I also added your test case as pn/kernel/test/auto/block.xml
Bert's analysis seems correct to me. With this change, we do end up
checking the container each time we call put(). However, without
this change, we get the bug you mention.
_Christopher
Bert Rodiers wrote:
> Hello Daniel,
>
> I looked at the model and the code and there might be a small difference in
> performance. It is true that in your case is there is a function call less,
> but in the current implementation of AbstractReceiver.putArray
> it is checked that the container of the the receiver is null or not and only
> in the case it is not null the conversion needs to be happen. Now this check
> happens for each array once. This distinction is probably still necessary
> and if you would switch the order of the loops the check needs to happen the
> size of the array times more (you could restructure the could so that you
> don't increase the times receiver.getContainer() is called, but you'll still
> need to check). Without doing actual performance measurements I however
> can't tell whether this will be an effect that is significant or not. I
> doubt it is.
> Another issue might be the number of times actors need to context-switch and
> block. If an actor consumes more than one token at a time from the same
> channel, it is blocked less, if tokens are send in a bunch at ones. However
> this is strongly dependent on the concrete model and it is difficult to make
> general statements about the performance.
>
> On the other hand, the current implementation (the one before your change)
> will result in larger buffer sizes for most models where there is a
> component whose output tokens are scattered and processed but a chain of
> actors and then the result is gathered again (similar to your model). The
> actual buffersize being needed depends on the model and can't be easily
> predicted (for your model 2 is enough).
>
> I think you can make the changes, but without actual performance critical
> models for which communication is the bottleneck it is hard to gauge this.
>
> Regards,
> Bert
>
>
> 2009/5/8 Daniel Crawl <crawl at sdsc.edu>
>
>> 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
>>>>
>
--
Christopher Brooks (cxh at eecs berkeley edu) University of California
CHESS Executive Director US Mail: 337 Cory Hall
Programmer/Analyst CHESS/Ptolemy/Trust Berkeley, CA 94720-1774
ph: 510.643.9841 fax:510.642.2718 (Office: 545Q Cory)
home: (F-Tu) 707.665.0131 (W-F) 510.655.5480
More information about the Kepler-dev
mailing list