jersey icon indicating copy to clipboard operation
jersey copied to clipboard

Getting rid of `new File(URL.getPath()).toURI()`

Open mkarg opened this issue 1 year ago • 3 comments

@jansupol Earlier this year you had to rework my NIO2 optimizations in Jersey due to problems with Windows path names. We found out that the original cause was not the Path class itself, but Jersey's use of URL.getPath(). In https://bugs.openjdk.org/browse/JDK-8314511 you claimed that you abstain from using Team OpenJDK's solution (URL.toURI()) because of performance reasons. Due to that I openend https://bugs.openjdk.org/browse/JDK-8321591 and checked the Jersey source code. In fact, I could not find a proof for a notable performance loss in Jersey's critical path, nor any other good reason for keeping new File(URL.getPath()).toURI(). As @AlanBateman explained several times, it is just a question of time until Jersey will run into a runtime fault as that code line will definitively fail with blanks in spaces (which is quite typical on Windows thanks to the Program Files folder for example). Hence I would kindly ask you to work with me to identify the actual reason why you still keep that "broken" code (and the risk of runtime failure), or to identify changes needed in OpenJDK to overcome your pretended performance shortcomings. Thanks! :-)

mkarg avatar Dec 10 '23 16:12 mkarg

The reason for filing the issue was not performance related at the beginning, the problem was that Jersey did not build on Windows. The issue showed the reason, Paths.get(MyTest.class.getResource("").getPath()); failed there and new File() fixes that.

The performance degradation comes from the additional operations the new File performs. getResource("").getPath() starts with /<drive>: and the linux-root slash at the beginning is normalized by new File(). But the NIO does not understand that linux-root slash on Windows.

jansupol avatar Dec 11 '23 12:12 jansupol

The reason for filing the issue was not performance related at the beginning, the problem was that Jersey did not build on Windows. The issue showed the reason, Paths.get(MyTest.class.getResource("").getPath()); failed there and new File() fixes that.

Hopefully it is clear now that MyTest.class.getResource("").getPath() returns the path component of the URL, it's not a file path.

The performance degradation comes from the additional operations the new File performs. getResource("").getPath() starts with /: and the linux-root slash at the beginning is normalized by new File(). But the NIO does not understand that linux-root slash on Windows.

On Windows, the new implementation will of course convert forward slashes to back slashes, the issue I think you are running into seems to be "/C:/" which comes about from treating the URL path component as a file path.

AlanBateman avatar Dec 11 '23 12:12 AlanBateman

@jansupol I would propose that I set up a test environment so I can check Alan's ideas. Regarding performance I do not see where this is in the critical path. Do you have a pointer for me where performance plays a role here, so I keep checking that special case then?

mkarg avatar Dec 11 '23 14:12 mkarg