Hardcoded default providers for asynchronous channel and file system

[Steve Poole]

On Fri, 2011-11-11 at 16:48 +0800, Jonathan Lu wrote:

I do think that Alan's approach is a more cross platform solution -
since to support a new platform one would only have to add in a new file
with the matching platform suffix.  The fact that some of these files
might be very similar could be dealt with with the right sort of
refactoring and having common, abstract parent classes.

Having said that I don't actually like having the platform name as a
suffix as it makes searching just a little bit more complicated.  I do
wish we could find a way to keep the .java suffix.  

What are the reasons for not making the platform (or equivalent) part of
the name?  - ie 

WindowsDefaultAsynchronousChannelProvider.java
LinuxDefaultAsynchronousChannelProvider.java
SolarisDefaultAsynchronousChannelProvider.java




> Hi Alan,
> 
> Thank you for reviewing.
> 
> Indeed my patch will bring some complexity to the build time, but I 
> think we'd better treat the three providers separately since they are 
> not of the exactly same type.
> 
> For DefaultAsynchronousChannelProvider, the logic for all the platforms 
> are the same except for provider's name. If use your approach, following 
> three template files will be created for each platform:
> DefaultAsynchronousChannelProvider.java.windows
> DefaultAsynchronousChannelProvider.java.linux
> DefaultAsynchronousChannelProvider.java.solaris
> But the only difference for them is one line containing the provider 
> class name, so will it be much clearer if we dynamically generate these 
> these files from one script or one template?
> 
> For DefaultFileSystemProvider, the logic for Linux and Solaris platforms 
> are a bit different from Windows platform.
> Class.forName() and Class.newInstance() are used to create 
> DefaultFileSystemProvider on Linux and Solaris, but only one simple new 
> statement found for Windows platform. Is there any particular reason to 
> place such a different logic here?
> I do not see any and think the same approach for 
> DefaultAsynchronousChannelProvider can also be applied here, please fix 
> me if incorrect.
> 
> On 11/10/2011 10:33 PM, Alan Bateman wrote:
> > On 08/11/2011 09:11, Jonathan Lu wrote:
> >>
> >> Hello Alan,
> >>
> >> Thanks for reminding! I've updated my patch to include 
> >> DefaultSelectorProvider, see the attachment.
> >> And I also tried to build the patched OpenJDK8 on Linux, Windows and 
> >> Solaris, the result seems OK.
> >> Could you please take a look at it?
> > I finally got a chance to look at this.
> >
> > My only concern with is that adds quite a bit of complexity to the 
> > build. From a portability point of view (the original objective) then 
> > it means editing three additional scripts for each platform too.
> >
> > What would you think about making it a lot simpler with something like:
> >
> > ifneq ($(PLATFORM),windows)
> >
> > $(GENSRCDIR)/sun/nio/fs/DefaultFileSystemProvider.java: \
> >     
> > $(PLATFORM_SRC)/classes/sun/nio/fs/DefaultFileSystemProvider.java.$(PLATFORM)
> >         $(install-file)
> >
> > clean::
> >         $(RM) $(GENSRCDIR)/sun/nio/fs/DefaultFileSystemProvider.java
> >
> > endif
> >
> > One benefit of this is that it would be trivial to change in the event 
> > that there were separate src/linux and src/solaris directories (BTW: 
> > don't read anything into this comment but at some point I think we 
> > should re-visit the current layout).
> >
> Okay :)
> But for current layout if adopt this approach, there'll be total 9 
> additional template files instead of 3 additional scripts. And the 
> templates of the same provider for different platforms are almost the same.
> > One other comment is that you've retained the code to choose the 
> > SelectorProvider implementation based on the Linux kernel. I think we 
> > should just drop this check as it's unlikely that anyone is porting 
> > and target ancient 2.4 kernels now.
> Yes, 2.6 or newer linux kernel should be the default assumption here 
> just like what the LinuxFileSystem did. So EPollSelectorProvider will be 
> the only result returned on Linux platform, PollSelectorProvider will 
> not be used any more.
> 
> But I do find a testcase  jdk/test/sun/nio/ch/SelProvider.java which 
> will test old Linux,  I think it should be changed accordingly, right?
> Moreover beside this test case, I do not see any other place where 
> PollSelectorProvider is used, so shall the PollSelectorProvider and 
> PollSelectorProviderImpl classes be completely removed from JDK8 repository?
> 
> After this kernel version change the logic of DefaultSelectorProvider 
> will be the same as DefaultAsynchronousChannelProvider.
> 
> 
> With above analysis, if DefaultSelectorProvider and 
> DefaultFileSystemProvider are all changed to the same logic as 
> DefaultAsynchronousChannelProvider, the condition here will be much simpler:
>        All the default provider classes for all three platforms are of 
> the same logic except for some naming differences!
> 
> In order to split the complexity out of scripts, what would you think 
> about composing a common template file for all providers and all the 
> platforms?
> all we need to do is to replace the place-holder names with actual 
> values for each provider and each platform. This approach will introduce 
> one template file or one script file :).
> 
> Regards!
> -Jonathan Lu
> 
> > -Alan.
>