Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Getting rid of new File(URL.getPath()).toURI() #5496

Open
mkarg opened this issue Dec 10, 2023 · 3 comments
Open

Getting rid of new File(URL.getPath()).toURI() #5496

mkarg opened this issue Dec 10, 2023 · 3 comments

Comments

@mkarg
Copy link
Member

mkarg commented Dec 10, 2023

@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! :-)

@jansupol
Copy link
Contributor

jansupol commented Dec 11, 2023

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.

@AlanBateman
Copy link

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.

@mkarg
Copy link
Member Author

mkarg commented Dec 11, 2023

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants