Community
Participate
Working Groups
We've got one single instance in WED & sametime from org.eclipse.update.core that is using FileChannel#transferTo() to copy a file. Would there be any chance we could get a change for 3.2GM for update.core to copy a file using standard streams? The NIO buffer APIs drag in ICU code that is a major problem for jcl platforms. I provide a patch.
Created attachment 38839 [details] patch for requested change Note, I did not test this patch in the Eclipse code context.
Phil, there is probably a reason why we used NIO class in this context. What is the performance downside of reverting to IO here?
I assumed it was used because it was convenient. If there is a performance concern, a better fix will be required to test for the NIO package first.
Dorian, I think you used NIO in certain places in Update. Was it for performance reasons or just for fun? Will the attached patch affect the performance in some way?
Branko, Konrad, do you remember why this particular section of code uses NIO?
yeah, I gotta imagine that the patch code will run slower but Phil's point in comment 3 was that the patch should then simply do a Class.forName("java.nio.FileChannel) and then depending on the outcome, either run the slow java.io code or the fast java.nio code. We have coded this pattern in several places (e.g., related to locking). Perhaps Phil can provide an updated patch to this effect?
No, as far as i know it was there before i came on board. Konrad or Dorian should know more
That's what happen when you have too much work on your plate, you don't remember what you did :-) Check this link: http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.update.core/src/org/eclipse/update/internal/core/JarDeltaInstallHandler.java
yes, i am sorry it is me. I will check it out
Created attachment 39116 [details] alternate patch to test for NIO classes Here is an alternate patch that tests for the NIO package and falls back to the standard stream method. Note this patch requires putting the NIO calls into another class to prevent the verifier from complaining about the original class. I'd still prefer to have the original patch if we can understand if there is really a performance issues in this code. Can someone explain the context this code get utilized? Note, I did not test this code in the update code context. The code was tested against jclMax 2.3.
Created attachment 39119 [details] updated patch to test for NIO classes Berthold pointed out I should remember class not found and avoid the exception more than once. Thanks Berthold. This is an updated patch that includes this.
Created attachment 39123 [details] final patch to test for NIO classes After more discussion, catching all other Exceptions would be bad as a null pointer would disable NIO copies in the previos patch. Testing shows that we can only get the errors at runtime. So this final patch catches just the errors and all other exceptions pass back to the caller.
You are making life way to hard. First, for NIO functionality as follows try { useNioCopy = Class.forName("java.nio.FileChannel") != null; } catch (ClassNotFoundException e) { useNioCopy = false; } Just do this once (e.g., in a static initializer. Then just write copyFile() and nioCopyFile() methods and use if (useNioCopy) nioCopyFile(...); else copyFile(...); clean and simple. The issue about having another class is interesting and the subject some debate around here. I just wrote a simple test that refrences a class in the true part of an if(). If I run the code such taht the condition is false, the class in the true part is not loaded. That is, I am not seeing a need to have a separate class. I wonder a) if I am missing something and am testing incorrectly b) there is a VM difference and on some VMs the class is actually loaded at verify time. Somewhat separate topic but any thoughts? In any event, if it really is true then the nioCopyFile can be done the way you have in the current patch. The nio existance check should still be simplified as it eliminates the need to catch some exceptions etc.
The patch supplied is very deliberate. jclmax will provide the locking methods from java.nio, but not the transfer and char buffer. I feel the patch supplied is not overly difficult and it covers all possible cases where the FileChannel class test does not. The seperate class for the NIO functions is required, no doubt. If it is not in a seperate class, the class verify will fail loading the JarDeltaInstallHandler because the class java.nio.channels.WritableByteChannel is not available (which is required because of the implicit type cast of the output FileChannel to WriteableByteChannel). I have verified that this is the only way the code will execute correctly. Now, I'd still prefer to just remove the NIO code completely. Anyone know the context this code gets executed?
Any chance of getting this into Eclipse 3.2 yet?
Ok, so it looks like this patch is the one that we want to put forward. The Update team should see what they think. Hopefully this can go in for RC2
Branko, please integrate the patch and do some sanity tests to see if everything is ok with it in place.
Need to be done for RC3
For what it's worth, I did fairly extensive performance testing of FileChannel-based copy versus stream-based copy, and I found no performance improvement (and a slight performance hit on Linux). I admit the implementation of file copying is much cleaner using FileChannel.transferTo, but if you're doing it for the performance, it doesn't buy you anything. See bug 124209 for details.
Sorry, looks like I slammed the priority field - changing back to P1.
Released.
Need formal votes. Dejan adds +1.
Jeff, need additional +1 before releasing.
Wassim, you can help Jeff to unblock this backlog. This bug is checked and ready to go.
+1 for RC3
fixed
