eclipse-platform / eclipse.platform

https://eclipse.dev/eclipse/
Eclipse Public License 2.0
83 stars 113 forks source link

Provide API for IFile Bulk read/write of all bytes/String/chars #1397

Closed jukzi closed 5 months ago

jukzi commented 6 months ago

Convenience methods like java.nio.file.Files.readAllBytes(Path) etc.

The new API could be used to substitute similar Util methods like org.eclipse.jdt.internal.core.util.Util.getResourceContentsAsCharArray (hotspot of jdt pasrsing) Searching the workspace like org.eclipse.search.internal.core.text.FileCharSequenceProvider.toShortString(IFile) is similar but not directly replaceable with this API.

Note that clients currently do have to query file.getCharset() and file.getContents() separately, possible opening the file twice to identify the UTF-BOM while the new API combines both calls, which would allow platform to open the file single time. While this PR does not implements such optimizations it is already faster to use the JDKs bulk methods instead of streaming.

Also this API avoids typical warnings of not closed Streams with the former stream api in client code.

mickaelistria commented 6 months ago

Can you please find some example of consumers in actual platform code and include replacement of older code with newer one in this PR so we can easily see the benefit of this new API over legacy and more easily discuss/review it?

github-actions[bot] commented 6 months ago

Test Results

 1 733 files   - 4   1 733 suites   - 4   1h 24m 1s :stopwatch: - 3m 0s  3 973 tests +1   3 951 :white_check_mark: +2   22 :zzz: ±0  0 :x:  - 1  12 506 runs   - 7  12 345 :white_check_mark:  - 6  161 :zzz: ±0  0 :x:  - 1 

Results for commit 2e1cae5e. ± Comparison against base commit 309a7f0d.

:recycle: This comment has been updated with latest results.

jukzi commented 6 months ago

Can you please find some example of consumers in actual platform code and include replacement of older code with newer one in this PR so we can easily see the benefit of this new API over legacy and more easily discuss/review it?

platform uses itself mainly in tests and never char[] There are some calls to org.eclipse.core.resources.IFile.getContents(boolean) with parameter false in the test. That has no new aequivalent. have refactored the rest, which already gives various examples how it can be used to simplify sourcecode.

jukzi commented 6 months ago

IFile is marked as @noExtend and @noImplement, so as long as default implementations don't save us code, I don't think defaults are necessary.

So far the theory. We know that not all clients obey such. As a default are an easy way there seems to be no need to prvent using default.

The nio-Files class also provides methods for reading/writings the lines of a (text)-file into/from a List/Stream. I find this often convenient and it has the advantage that not a large byte-array/String has to be created.

I am not aware of any such code actually used in any hotspot, so i do not see a need for that. Still you could suggest such methods in a separate PR.

As a in principle different approach we could also think about providing a custom nio-FileSystem that reflects Eclipse's workspace file-system and corresponding IPath implementations. Maybe IPath could then just extend nio-Path. This would have the advantage that we can just use the standard Java Files API and would benefit from potential future enhancements.

That would be really cool, but needs intensive performance testing and my first guess is that the IPath to Path conversions may be a performance or memory bottle neck. I do not plan to work on that by now.

jukzi commented 6 months ago
  • [ ] API tools complains about wrong since

i guess i have to fix that after master is open with appropriate bundle version updates.

  • [ ] Compiler complains about usage of deprecated methods

That may not be avoidable as most of the code is copy and paste of existing code that already has such warnings. This PR is not about to change that but to keep code consistent with the existing implementation.

laeubi commented 6 months ago

That may not be avoidable as most of the code is copy and paste of existing code that already has such warnings.

Please just fix that, if we "copy & paste" Warnings this does not help in any way, if there is no easy alternative the warning must be suppressed, in most cases it is simply that the name changed or another call should be used instead.

jukzi commented 5 months ago

CI complains with warnings like Your build strictly depends on unit org.eclipse.core.resources.source 3.20.200.v20240513-1323 that is shadowed by a reactor project, this can lead to unexpected build results! Similar warnings did already exist. can that be ignored?

jukzi commented 5 months ago

All checks passed and i have solved all review comments as far as it made sense. Please finish your reviews.