[kepler-dev] [Ptolemy] AbstractReceiver.java
Daniel Crawl
crawl at sdsc.edu
Tue May 12 11:54:18 PDT 2009
Hi Bert and Christopher,
Thanks for clarifying the performance problem, and adding
my update and test case.
What do you think of the following putArrayToAll? I think
this is what Bert suggested; it does not make extra calls
to getContainer.
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.");
}
// Cache the containers for each receiver to minimize
// the number of calls to getContainer.
IOPort[] containers = new IOPort[receivers.length];
for (int j = 0; j < receivers.length; j++) {
containers[j] = receivers[j].getContainer();
}
// 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++ ) {
if (containers[j] == null) {
receivers[j].put(tokens[i]);
} else {
receivers[j].put(containers[j].convert(tokens[i]));
}
}
}
}
Thanks,
--dan
Christopher Brooks wrote:
> 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
>>>>>
>>
>
More information about the Kepler-dev
mailing list