Selector cleanup

[Alan Bateman]

Rémi Forax wrote:
> Hi all, i currently develop a small web server and  I think codes related
> to selectors can be improved just by changing some small pieces of code.
> To be crystal clear, i don't want to re-implement all selector related 
> stuffs but
> just patch some parts of the actual code.
>
> There are some allocations in JDK API  that can be removed,
> the code was badly retrofited to 1.5 and lot of field can be declared 
> final.
> Some methods/fields still 'use' raw types and doesn't take
> advantage of autoboxing.
You're right. Much of the code here dates back to 1.4 and we haven't 
gone back to clean-up things like this.

>
> Futhermore, there is some divergence between Windows and *nix
> code i don't understand.
> By example, WindowsSelectorImpl and PollSelectorImpl uses a pipe to
> implements wakeup but WindowsSelectorImpl  relies on Pipe
> and PollSelectorImpl on IOUtil.initPipe().
> I think this code should be the same.
Ideally we would use a socketpair for the wakeup mechanism but Windows 
doesn't support it. For this reason, Pipe is implemented as a loopback 
connection and this works okay for the wakeup mechanism too. One thing 
to mention is that PollSelectorImpl is only used now when running on the 
Linux 2.4 kernel (it's not used with the 2.6 kernel and isn't used on 
Solaris). I just mention this as someday it might become obsolete and we 
can remove it.

>
> in WindowsSelectorImpl:
>  - updateSelectedKeys() use an iterator to traverse the array
>    (an ArrayList). It should use an indexed loop instead
>    to avoid Iterator allocation.
>  - field threads should be declared as an ArrayList
>    because adjustThreadsCount() supose that i can be iterate
>    using an indexed loop.
>    Furthermore, it can be generified like this:
>    private final ArrayList<Thread> threads = new ArrayList<Thread>();
These clean-ups seem reasonable.

> - class FDMap,
>   I don't see why FdMap need to be a class, all methods can be moved
>   as member methods of WindowsSelectorImpl without problems.
>   Futhermore, the constructor of FdMap is private (get/put/remove too)
>   so the compiler stupidly inserts accessor methods (access$000 etc.).
>   Ok, the main point, here when the code was retrofited to 1.5,
>   The new Integer() was not transformed to use Integer.valueOf()
>   to share small integers and avoid allocation if file descriptor 
> value are small.
These are SOCKET types rather than file descriptors and unlikely to be 
in the range that Integer caches (actually it should be a Long but that 
is a story for another day).

> - In class MapEntry, ski should be declared final.
> - close(), set selectedKeys() to null doesn't allow the Set to be 
> collected
>    because publicSelectedKeys contains() a reference to it.
>
> in PollSelectorImpl:
>  - interruptLock should be final.
>  - close(), see WindowsSelectorImpl
>
> in EpollSelectorImpl:
>  - like in poll, interruptLock should be final.
>  - hashMap fdTokey should be generified and final.
>  - close(), see WindowsSelectorImpl
>  - implRegister/implDereg
>    - They should use Integer.valueOf() instead of new Integer().
>    - IOUtil.fdVal() is used spuriously, in implRegister but not
>      in implDereg.
These are integers so there could be some benefit (but probably very 
hard to measure).

>
>  - EPollArrayWrapper
>    - updateList is a LinkedList, a double linked list that stores 
> Updator object,
>      I think it's more efficient to add a field next in the Updator 
> object and
>      link updator by hand in order to avoid to create LinkedList$Entry .
Maybe but probably very hard to measure.

>   - Updataor.opcode and Updator.fd should be declared final.
>     - SelectorImpl:
>     key and selectedKeys should be LinkedHashSet instead of Set
>     because they are frequently iterated.
>   let discuss about that before I submit patchs.
The clean-ups you suggest seem reasonable so I would suggest going ahead 
and sending a patch. I'm happy to review and work with you to get the 
clean-ups integrated (once OpenJDK/jdk7 re-opens for changes of course).

-Alan.

PS: I don't know anything about your "small web server" but the simple 
server in com.sun.net.httpserver may be useful.