Salut,
Oleksiy Stashok wrote:
> Hi,
>
> here is new propose for CONNECT stuff.
> I propose to introduce new interface (it could be better to add
> additional method to CallbackHandler, but don't want to break existing
> API) CallbackHandlerDescriptor, which currently has just one method:
> isExecuteInSeparateThread(Op).
Good idea to not break anybody now :-) I agree it would have been much
better to add a new API.
> And when preparing to notify CallbackHandler, TCPSelectorHandler checks
> if callbackHandler implements CallbackHandlerDescriptor interface, if
> yes - checks if current CallbackHandler operation should be run in
> current or separate thread.
> This way, from one side existing apps will work the same way there
> worked, and we will have ability control CallbackHandler method
> invocation in cases, where we will need that.
OK looks good. I'm just afraid of the performance of invoking instanceof
on every operations :-(, but I don't have a better idea to propose....
+1
-- Jeanfrancois
>
> Thanks.
>
> WBR,
> Alexey.
>
> Jeanfrancois Arcand wrote:
>> Salut,
>>
>> Oleksiy Stashok wrote:
>>> Hi,
>>>
>>>>>>> think it could be good idea from perf. side, but only if it will
>>>>>>> be used right way.
>>>>>>>
>>>>>>> We have to be very careful with that change! Because if someone
>>>>>>> will have complex logic in CallbackHandler.onConnect - it will
>>>>>>> lead to big perf. problems.
>>>>>>> For example - SSLConnectorHandler usage. Currently, in our
>>>>>>> samples from time to time we execute *blocking*
>>>>>>> sslConnectorHandler.handshake directly from onConnect method,
>>>>>>> which definitely should not happen anymore!
>>>>>>
>>>>>> Agree this is not good for TLS as this operations is quite slow. I
>>>>>> would think we should made it configurable then, and let the
>>>>>> current default to spawn a thread for TLS, but not for TCP....but
>>>>>> that might be quite confusing. Not sure what is the best solution...
>>>>> What about making current mode as default, and make it configurable
>>>>> by adding TCPSelectorHandler.is/setConnectOnSeparateThread() ?
>>>>> Because I'm not sure if it will be more clear, if we will have
>>>>> different default behaviors for TCP and SSL.
>>>>>
>>>>> What do you think?
>>>>
>>>> OK make sense. What about UDPSelectorHandler? I would think we need
>>>> to promote the method on the ConnectorHandler so both implementation
>>>> can turn it on/off
>>> With UDPSelectorHandler we have different situation. Currently our
>>> UDP implementation tries to behave the same or almost the same way
>>> TCP does. But actually for UDP we don't need all that connect stuff.
>>> As UDP channel is connected just after DatagramChannel.connect(..) is
>>> called. So for UDP there is no need at all in
>>> CallbackHandler.onConnect(...).
>>> On other side, after executing UDPConnectorHandler.connect(...) we
>>> can not be sure that datagram channel is registered on selector, we
>>> can just schedule registration task, which will be executed on
>>> selector thread.
>>> But is it required for us to be notified when datagram channel
>>> becomes registered with selector? Think for most of scenarios this
>>> info is not required.
>>
>> OK so then what about adding a new interface that only have
>> onRead/onWrite method and make CallbackHandler extending that
>> interface with the onConnect method (so current applications aren't
>> broken/still compatible) and handle the case inside the
>> SelectorHandler? I think we can extends both TCP and UDP, where for
>> TCP we do the minimal operations like registering for OP_READ and
>> OP_WRITE?
>>
>>
>>> If we will remove TCP-like connect behavior from UDP implementation -
>>> we can improve performance, as we will not need to redundant context
>>> switches and return connected UDPConnectorHandler instantly.
>>>
>>> What do you think?
>>
>> +1.
>>
>> Let me work on some real implementation :-)
>>
>> A+
>>
>> -- jeanfrancois
>>
>>
>>>
>>> WBR,
>>> Alexey.
>>>>
>>>> A+
>>>>
>>>> -- jeanfrancois
>>>>
>>>>>
>>>>> WBR,
>>>>> Alexey.
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> In our javadoc we should warn about such *new default* behavior :))
>>>>>>>
>>>>>>> BTW, We can try to implement this more common way (attached)? :)
>>>>>>
>>>>>> LOL :-) Yes I prefer your patch :-) What about exposing the
>>>>>> runInSeparateThread properties so it can be changed directly when
>>>>>> using the SelectorHandler? The bad side is it will require bumping
>>>>>> the release version as we are introducing a new API....
>>>>>>
>>>>>> A+
>>>>>>
>>>>>> -- Jeanfrancois
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Thanks.
>>>>>>>
>>>>>>> WBR,
>>>>>>> Alexey.
>>>>>>>
>>>>>>> Jeanfrancois Arcand wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> we recently faced a bug in Sailfin where performance
>>>>>>>> significantly decreased (cut&paste from Scott update on the bug):
>>>>>>>>
>>>>>>>>> This is not so much lock contention as it is a form of deadlock
>>>>>>>>> that happens
>>>>>>>>> under load. What happens is that all the SipContainer threads
>>>>>>>>> attempt to write
>>>>>>>>> to a socket (the sipp uas process); one of them will open the
>>>>>>>>> socket while the
>>>>>>>>> rest of them block waiting to enter the same code. The thread
>>>>>>>>> that opens a
>>>>>>>>> socket then waits for the socket connection to be completed.
>>>>>>>>>
>>>>>>>>> The completion of the socket connection is signaled by the
>>>>>>>>> TCPSelectorThread,
>>>>>>>>> which creates a callback task and executes it on the
>>>>>>>>> SipContainer thread pool.
>>>>>>>>> Since all of those threads are busy (blocked waiting to enter
>>>>>>>>> the socket-writing
>>>>>>>>> code), the callback isn't ever processed, and the stack trace
>>>>>>>>> below results.
>>>>>>>>> Eventually, the thread waiting for the callback will time out,
>>>>>>>>> and things will
>>>>>>>>> run for a few seconds until the same state is reached (and
>>>>>>>>> eventually, there
>>>>>>>>> will be sockets created and things will run okay).
>>>>>>>>>
>>>>>>>>> A possible fix for sailing is to override the TCPSelectorThread
>>>>>>>>> and execute the
>>>>>>>>> onConnect callback directly rather than putting that on another
>>>>>>>>> thread.
>>>>>>>>
>>>>>>>> I would like to propose that we change the current behavior and
>>>>>>>> avoid executing the OP_CONNECT on its own thread, but instead
>>>>>>>> executing using the SelectorHandler thread. This is what we are
>>>>>>>> already doing for OP_ACCEPT based on some performance measure we
>>>>>>>> did for Grizzly 1.0, and based on Scott's investigation, I would
>>>>>>>> like to change the default behavior
>>>>>>>>
>>>>>>>>> Index: TCPSelectorHandler.java
>>>>>>>>> ===================================================================
>>>>>>>>>
>>>>>>>>> --- TCPSelectorHandler.java (revision 712)
>>>>>>>>> +++ TCPSelectorHandler.java (working copy)
>>>>>>>>> @@ -697,8 +697,13 @@
>>>>>>>>> try {
>>>>>>>>> CallbackHandlerContextTask task =
>>>>>>>>> CallbackHandlerContextTask.poll();
>>>>>>>>> task.setCallBackHandler(callbackHandler);
>>>>>>>>> - context.execute(task);
>>>>>>>>> - } catch (PipelineFullException ex){
>>>>>>>>> + if (context.getCurrentOpType() !=
>>>>>>>>> Context.OpType.OP_CONNECT){
>>>>>>>>> + context.execute(task);
>>>>>>>>> + } else {
>>>>>>>>> + task.setContext(context);
>>>>>>>>> + task.call();
>>>>>>>>> + }
>>>>>>>>> + } catch (Exception ex){
>>>>>>>>> throw new IOException(ex.getMessage());
>>>>>>>>> }
>>>>>>>>> }
>>>>>>>>
>>>>>>>>
>>>>>>>> If someone rely on the current behavior (spawning a Thread),
>>>>>>>> then the TCPSelectorHandler can always be extended to act like
>>>>>>>> previous version.
>>>>>>>>
>>>>>>>> I'm interested to learn from the Corba team about how OP_CONNECT
>>>>>>>> has been handled (if that was used).
>>>>>>>>
>>>>>>>> A+
>>>>>>>>
>>>>>>>> -- Jeanfrancois
>>>>>>>>
>>>>>>>> ---------------------------------------------------------------------
>>>>>>>>
>>>>>>>> To unsubscribe, e-mail: dev-unsubscribe_at_grizzly.dev.java.net
>>>>>>>> For additional commands, e-mail: dev-help_at_grizzly.dev.java.net
>>>>>>>>
>>>>>>>
>>>>>>> ------------------------------------------------------------------------
>>>>>>>
>>>>>>>
>>>>>>> ---------------------------------------------------------------------
>>>>>>>
>>>>>>> To unsubscribe, e-mail: dev-unsubscribe_at_grizzly.dev.java.net
>>>>>>> For additional commands, e-mail: dev-help_at_grizzly.dev.java.net
>>>>>>
>>>>>> ---------------------------------------------------------------------
>>>>>> To unsubscribe, e-mail: dev-unsubscribe_at_grizzly.dev.java.net
>>>>>> For additional commands, e-mail: dev-help_at_grizzly.dev.java.net
>>>>>>
>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: dev-unsubscribe_at_grizzly.dev.java.net
>>>>> For additional commands, e-mail: dev-help_at_grizzly.dev.java.net
>>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: dev-unsubscribe_at_grizzly.dev.java.net
>>>> For additional commands, e-mail: dev-help_at_grizzly.dev.java.net
>>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe_at_grizzly.dev.java.net
>>> For additional commands, e-mail: dev-help_at_grizzly.dev.java.net
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe_at_grizzly.dev.java.net
>> For additional commands, e-mail: dev-help_at_grizzly.dev.java.net
>>
>
> ------------------------------------------------------------------------
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe_at_grizzly.dev.java.net
> For additional commands, e-mail: dev-help_at_grizzly.dev.java.net