Jeroen Frijters wrote:
> Casey Marshall wrote:
>> What I'd like people to look at and comment on specifically are the
>> changes to gnu.java.nio.VMChannel and gnu.java.net.VMPlainSocketImpl,
>> for opinions on how well these classes (the public methods) help
>> abstract away platform-specific code.
>
> Thanks. I like the idea of the patch, but I have quite a few comments
> about the current implementation. As a reminder: Please don't take my
> directness the wrong way, English is not my native language and I'm
> rather blunt, even for a Dutchman.
>
These are all excellent points! Thanks for looking at this.
> Here are most of them:
>
> - The NativeFD interface should have a better name (e.g. something like
> VMChannelOwner).
Agreed. I can't think of a good adjective for a name, though (but
VMChannelOwner is fine).
> - I don't like the fact that you're introducing allocation in virtually
> every I/O path.
Hmm, I don't think I'm doing that as much for Channels, where
performance is arguably more important.
> - Why "vmch.getNativeFD().close();" instead of "vmch.close();"?
No reason. I wanted the `close' bit to be tied to the NativeFD object,
but putting the API close call in the containing class is fine.
> - Most classes should not have a finalize method, only VMChannel should
> have a finalize method.
Yeah, true (all finalize methods did exist previously; I just changed
what they did instead of removing them).
> - All valid socket options should be delegated to impl (and if any
> aren't yet implemented, the impl should throw the
> UnsupportedOperationException).
Yes.
> - Method naming could be better ("return
> ch.getVMChannel().getNativeFD().getNativeFD();").
Agreed :-)
> - Instead of changing FileInputStream/FileOutputStream/RandomAccessFile,
> FileChannelImpl should accommodate them.
I'm not sure what you mean by this.
> - PlainSocketImpl uses ByteBuffer.allocateDirect to allocate the two
> single byte buffers (oneByteRead and oneByteWrite, which aren't used
> BTW), I'd like to see the decision to use allocate vs. allocateDirect
> delegated to VMPlainSocketImpl. (For me allocateDirect is actually less
> efficient in this case).
Yeah, that was some cruft for an idea that didn't pan out. Also, I
typicaly used allocateDirect because I (mistakenly) thought that the
scatter/gather IO methods required direct buffers (they don't). I didn't
correct this everywhere.
And, our ByteBuffers in general suffer from some performance issues.
> - PlainSocketImpl.connect should simply call
> impl.connect(address,timeout) which in turn calls
> channel.connect(address,timeout), the decision how to implement the
> timeout should be in the VM layer.
Good point.
> - PlainSocketImpl.write(byte[],int,int) implementation is unacceptable.
>
OK. I don't think that method is needed any more, though. The output
stream calls on the channel directly.
Thanks for the feedback!
