Selector cleanup

[Rémi Forax]

Alan Bateman a écrit :
> 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.
ok.
>
>>
>> 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).
ok, no valueOf(), i'm not an expert in Windows API.
But are you agree that class FdMap is not necessary.
>
>> - 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).
yes, very hard to mesure until you span 1k thread with one selector each.
btw if you take a look to EPollArrayWrapper, idlSet already use boxing.
>
>>
>>  - 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 will do that.
> I'm happy to review and work with you to get the clean-ups integrated 
> (once OpenJDK/jdk7 re-opens for changes of course).
Do you have any idea when openjdk will be reopen ?
>
> -Alan.
>
> PS: I don't know anything about your "small web server" but the simple 
> server in com.sun.net.httpserver may be useful.
My small server is a research project that embeds a non-blocking parser 
in a webserver and
claims to have the same performance than grizzly. I will post a blog 
entry about it
when all benchmarks will be finished.

Rémi