Great catch!
I've removed the copyTo() in DefaultSelectionKeyHandler.
I've also completed Jeanfrancois's suggested changes.
Anyone else have any further comments?
If not, I think I am ready to commit.
charlie ...
Oleksiy Stashok wrote:
> Hello,
>
> +1 for that change!
> Small note :)
> IMHO we can remove copyTo method from DefaultSelectionKeyHandler,
> right, there is no reason to override it?
>
> Thanks.
>
> WBR,
> Alexey.
>
> On 19.02.2008, at 17:53, charlie hunt wrote:
>
>> Zapping some of the non-relevant stuff to shrink message size.
>>
>> Responses embedded below.
>>
>> Jeanfrancois Arcand wrote:
>>> charlie hunt wrote:
>>>> ...[other stuff zapped]...
>>>>
>>>> 2.) I am not 100% happy with the refactoring since the
>>>> SelectionKeyHandler interface implies expiring keys with its
>>>> expire() method. And, this notion also propagates into
>>>> TCPSelectorHandler.postSelect(). To be 100% happy, a
>>>> SelectionKeyHandler interface and TCPSelectorHandler.postSelect()
>>>> would need to be refactored. I don't have a problem with
>>>> refactoring TCPSelectorHandler, but I don't like the idea of
>>>> refactoring SelectionKeyHandler. I would summarize the issue here
>>>> as; the notion of expiring keys should be a more general "post
>>>> select" activity and not forced upon a user of a Grizzly transport
>>>> when he / she does not need expiring keys support. We might
>>>> consider addressing this in Grizzly 2.0 also.
>>>
>>> Interesting :-) Can you file an issue so we don't forget it?
>>
>> Yes. I am planning to submit an issue and also reference it the
>> issue I have filed for this enhancement work.
>>
>>>
>>> ... [stuff zapped] ...
>>>
>>> Comment inline...
>>>
>>> ... [stuff zapped] ...
>>>>
>>>> Index: main/java/com/sun/grizzly/TCPSelectorHandler.java
>>>> ===================================================================
>>>> --- main/java/com/sun/grizzly/TCPSelectorHandler.java (revision
>>>> 840)
>>>> +++ main/java/com/sun/grizzly/TCPSelectorHandler.java (working
>>>> copy)
>>>> @@ -407,7 +407,7 @@
>>>> SelectionKey.OP_CONNECT);
>>>>
>>>> key.attach(CallbackHandlerSelectionKeyAttachment.create(key,
>>>> callbackHandler));
>>>> - boolean isConnected = false;
>>>> + boolean isConnected;
>>>
>>> Stupid question. Not initializing the variable makes a difference?
>>
>> No such thing as a stupid question. ;-)
>>
>> It "can" make a difference. It depends on the usage and what else
>> the JIT compiler observes in the usage of the application.
>>
>> It may in some cases make no difference. In some situations the JIT
>> compiler may see that you are loading value into that variable before
>> ever using the initialized value and essentially remove the initial
>> "load" as an optimization. In other cases, it may not do that kind
>> of optimization (remove the initial load). The only way to "really"
>> know is to look at the generated code.
>>
>> It can be an issue if; the code resulting in a load instruction is
>> on a really hot" path. But, since this specific case is an
>> OP_CONNECT, it's likely this is will not a "big" difference, or a
>> noticeable difference.
>>
>> I've just gotten into the habit of pro-actively removing "unnecessary
>> loads" whenever I see them so they don't have an opportunity to
>> become a problem.
>>
>> Btw, I'm not a JIT compiler expert. There's a lot of "rocket
>> science" that goes on in the compiler and about the only I can really
>> find out answers to what it is doing is look at the generated code in
>> Sun Studio Collector / Analyzer in "Machine Mode". It's kind of a
>> tedious task. But, interesting nonetheless.
>>
>>> ... [other stuff zapped] ...
>>>
>>> Some description :-)
>>
>> I'll add it.
>>
>>>> */
>>>> public class BaseSelectionKeyHandler implements SelectionKeyHandler {
>>>>
>>>> ... [stuff zapped] ...
>>>>
>>>> @SuppressWarnings("empty-statement")
>>>> protected void closeChannel(SelectableChannel channel) {
>>>>
>>>> if (channel instanceof SocketChannel) {
>>>> Socket socket = ((SocketChannel) channel).socket();
>>>> try{
>>>> if (!socket.isInputShutdown()) socket.shutdownInput();
>>>> } catch (IOException ex){
>>>> ;
>>>> }
>>>> try{
>>>> if (!socket.isOutputShutdown())
>>>> socket.shutdownOutput();
>>>> } catch (IOException ex){
>>>> ;
>>>> }
>>>> try{
>>>> socket.close();
>>>> } catch (IOException ex){
>>>> ;
>>>> }
>>>> }
>>>> try{
>>>> channel.close();
>>>> } catch (IOException ex){
>>>> ; // LOG ME
>>>> }
>>> * @author Charlie Hunt
>>>
>>> Since we are refactoring, what do you think about logging all
>>> IOException here to FINEST?
>>
>> Agreed. I was considering doing this. So, I'd be glad to do it.
>>
>> charlie ..
>>
>>>
>>> Thanks!
>>>
>>> -- Jeanfrancois
>>>
>>>
>>>> }
>>>> }
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe_at_grizzly.dev.java.net
>>> For additional commands, e-mail: dev-help_at_grizzly.dev.java.net
>>>
>>
>> <charlie.hunt.vcf>---------------------------------------------------------------------
>>
>> 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
>
--
Charlie Hunt
Java Performance Engineer
<http://java.sun.com/docs/performance/>