Repository navigation
Conversation
51fda51 to
52c1978
Compare
52c1978 to
777f935
Compare
|
On the long run it would probably make most sense to use FFM under Linux/POSIX only to implement the The existing native implementations to fetch/put file information are intended to be removed via If you complete a FFM based implementation of
Since #2908 is now merged, these errors are now warnings already. Furthermore it's correct that the generated code is not the most compact one. But I think deterministically generated code is better to maintain on the long run, than AI generated one. |
777f935 to
c9c2df0
Compare
|
Thanks, I have switched the bindings to jextract. One exception: statx is bound by hand from the generated statx$address() and statx$descriptor(), with Linker.Option.captureCallState("errno") added, since jextract does not emit that option. Without errno every failure looks the same and RefreshLocalVisitor deletes workspace resources whose statx failed with EACCES or EIO. The JNI handler reads errno today, so dropping the distinction would be a regression rather than a new limitation. That is similar to your adjustment in Lines 113-137 of the merged Win32Handler. Since #2925 would change native code, I would rather not have both of us editing PosixHandler and LocalFileNativesManager at the same time: I can follow-up once #2925 is in. |
|
@HannesWell how did you handle the additional warnings from the jextract generated code in your windows implementation? Just reset the quality gate? (How do I do this?) |
0249e6f to
4fe0585
Compare
|
I added a clean-up step to get rid of the additional warnings, see https://github.com/eclipse-platform/eclipse.platform/pull/2926/changes#diff-17d9390d9a5b073c8b584098a9193edeb880d06d3d86da553ccc9a1f5baa3318 |
HannesWell
left a comment
HannesWell
left a comment
There was a problem hiding this comment.
Since #2925 would change native code, I would rather not have both of us editing PosixHandler and LocalFileNativesManager at the same time: I can follow-up once #2925 is in.
Thanks for your patience. That change currently has a problem for macOS but I'm working on a solution. Will let you know once it's completed.
how did you handle the additional warnings from the jextract generated code in your windows implementation? Just reset the quality gate? (How do I do this?)
Yes, exactly I reset the quality gate. In Jenkins there apers a Reset button in the overview page of a build next to the failed quality gates (usually a bit down the page).
I added a clean-up step to get rid of the additional warnings, see https://github.com/eclipse-platform/eclipse.platform/pull/2926/changes#diff-17d9390d9a5b073c8b584098a9193edeb880d06d3d86da553ccc9a1f5baa3318
That should work. Alternatively, you could also just add @SuppressWarnings("unused") annotations to the class or maybe just suppress all warnings.
Since the generated files are pretty large now and we want to trim the code to what's actually used (automatically) eventually. I suggest to remove unused code now already to avoid adding thousands of lines of code to git now only to later remove it.
On the long run it would probably make most sense to use FFM under Linux/POSIX only to implement the
NativeHandlermethodslistDirectoryNames()andlistDirectoryAndGetFileInfos()in the existingPosixHandler. All other methods can probably be implemented using Java NIO with comparable performance.
Furthermore I more and more believe we should just do this immediately with this PR.
And just implement listDirectoryAndGetFileInfos() in the PosixHandler now, i.e. with this PR.
A temporarily introduced separate handler probably doesn't have much users anyway.
That would hopefully also reduce the generated code.
I also challenge that listDirectoryNames() requires a native implementation and wonder if the simple Java implementation is comparable fast, as asked in
| cat > /tmp/LibC.h <<'HEADER' | ||
| #define _GNU_SOURCE | ||
| #include <dirent.h> | ||
| #include <errno.h> | ||
| #include <fcntl.h> | ||
| #include <limits.h> | ||
| #include <linux/stat.h> | ||
| #include <sys/stat.h> | ||
| #include <unistd.h> | ||
| HEADER |
There was a problem hiding this comment.
As far as I know jextract can handle multiple header files at once.
So creating this temp header file is probably not necessary and you could just list the headers one by one as arguments to jextract.
Furthermore you can also define macros using:
-D --define-macro <macro>=<value> define <macro> to <value> (or 1 if <value> omitted)
|
Almost all of it is statx.java (about 1600 lines), because jextract can't limit a struct to certain fields. Deleting accessors by hand would make the output no longer reproducible from the script, which works against the reason for using jextract. To make this reproducable I will put it in the scriptto strip the unused accessors after generation, in the same way it already removes unused imports. @SuppressWarnings doesn't reach unused imports, because JDT reports those outside the class. That's why the script deletes them and adds @SuppressWarnings("all") to each class for the rest. set -eu we should keep because shebang flags are ignored when the script is run as sh generateLinuxH.sh, and set -eu always applies. I try to check listDirectoryNames() performance and it if is fine compared to the FFM version, I use it. |
PosixHandler lists a directory and then asks java.nio for the attributes of every entry by its full path. On Linux it now reads the directory with opendir and readdir and stats each entry relative to it with statx, through the Foreign Function & Memory API. The layout of struct statx is the same on every architecture, so this needs no compiled library, and the results match the java.nio path exactly, including for broken and self referencing symbolic links. Listing 355984 entries in 36444 directories takes 414 ms against 670 ms for java.nio and 523 ms for the JNI LinuxFileHandler. The bindings are generated by jextract with the script added here, which drops the generated members nothing refers to. statx is bound by hand from the generated descriptor and symbol, because reading errno needs captureCallState, which jextract does not emit, and errno is what tells a missing file from one that cannot be read. Assisted-by: multiple AI agents and layers of automated tooling 🤖
4fe0585 to
c95e327
Compare
vogella
changed the title
|
Any additional feedback? Planning to merge tomorrow |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Embedded NULs, readdir and readlinkat errors, and runtime statx unavailability require correct handling before approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Adds an FFM-based Linux directory reader to improve bulk file-attribute retrieval.
Changes:
- Integrates
opendir,readdir, andstatx. - Adds generated Linux libc bindings and regeneration tooling.
- Connects the optimized reader to
PosixHandler.
| File | Description |
|---|---|
resources/bundles/org.eclipse.core.filesystem/src/org/eclipse/core/internal/filesystem/local/nio/PosixHandler.java |
Selects the Linux FFM reader. |
resources/bundles/org.eclipse.core.filesystem/src/org/eclipse/core/internal/filesystem/local/nio/LinuxDirectoryReader.java |
Implements directory enumeration and metadata retrieval. |
resources/bundles/org.eclipse.core.filesystem/src-gen/org/linux/statx.java |
Defines the statx structure layout. |
resources/bundles/org.eclipse.core.filesystem/src-gen/org/linux/statx_timestamp.java |
Defines statx timestamp accessors. |
resources/bundles/org.eclipse.core.filesystem/src-gen/org/linux/LibC$shared.java |
Provides shared native layouts. |
resources/bundles/org.eclipse.core.filesystem/src-gen/org/linux/LibC.java |
Binds required libc functions and constants. |
resources/bundles/org.eclipse.core.filesystem/src-gen/org/linux/dirent.java |
Defines the directory-entry layout. |
resources/bundles/org.eclipse.core.filesystem/generateLinuxH.sh |
Regenerates and prunes Linux bindings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| static IFileInfo[] listDirectoryAndGetFileInfos(String fileName) { | ||
| Scratch scratch = Scratch.CURRENT.get(); | ||
| MemorySegment dir = LibC.opendir(scratch.path(fileName)); | ||
| if (dir.address() == 0) { | ||
| return NO_INFOS; |
iloveeclipse
left a comment
iloveeclipse
left a comment
There was a problem hiding this comment.
I disagree with the main direction of this change. "Nio" code / handler is supposed to provide "nio"-only implementation, as a fallback for any "native" implementation which might not work on some platform. Mixing now FFM into NIO is nkt the right way. We should get rid of problems we have (3 different native implementations which are hard ro maintain in C/Java mixed code, not create new problems by mixing FFM into non-native parts of code.
There was a problem hiding this comment.
- Why so strange package name? org.linux ? Please use proper namespace.
- Why it is in src-gen folder? It is confusing because such folders usually do not contain checked in code.
- License header is missing.
|
|
||
| /** | ||
| * Lists a directory together with the attributes of its entries on Linux, with | ||
| * one {@code readdir} and one {@code statx} per entry through the Foreign |
There was a problem hiding this comment.
File is placed under "nio" package but says it uses FFM instead of nio. Makes no sense?
| * The native buffers these calls need, held per thread and reused, so that a | ||
| * listing allocates nothing per entry beyond its {@link FileInfo}. | ||
| */ | ||
| private static final class Scratch { |
There was a problem hiding this comment.
What "Scratch" type name means in this context?
| @Override | ||
| public IFileInfo[] listDirectoryAndGetFileInfos(String fileName) { | ||
| if (LinuxDirectoryReader.isAvailable()) { | ||
| return LinuxDirectoryReader.listDirectoryAndGetFileInfos(fileName); |
There was a problem hiding this comment.
Why is this call here from "pure" NIO code to FFM based? It produces spaghetty like dependencies which are not obvious to detect/understand.
I would also prefer to have a cleaner approach here, not mixing FFM migration with additional changes as I originally did. |
|
For the future work in this area, some thoughts / wishes from me:
|
HannesWell
left a comment
HannesWell
left a comment
There was a problem hiding this comment.
Deleting accessors by hand would make the output no longer reproducible from the script, which works against the reason for using jextract. To make this reproducable I will put it in the scriptto strip the unused accessors after generation, in the same way it already removes unused imports.
Agree. It was just meant as an intermediate anticipation to avoid adding code now, that's deleted later.
Adding a bash script is fine more (I'd say it's not the ideal tool for that, but if it works that's nice to have for now).
However I checked the code and some methods are still unused.
@SuppressWarnings doesn't reach unused imports, because JDT reports those outside the class. That's why the script deletes them and adds @SuppressWarnings("all") to each class for the rest.
I cannot confirm that. When applying @SuppressWarnings("all") to a class even unused import warnings are gone for me.
set -eu we should keep because shebang flags are ignored when the script is run as sh generateLinuxH.sh, and set -eu always applies.
Acknowledged.
Update applied
I personally try to avoid dismissing request for changes from reviews, but instead ask for a subsequent review. I'd consider a dismiss similar to a rejection.
| * listing allocates nothing per entry beyond its {@link FileInfo}. | ||
| */ | ||
| private static final class Scratch { | ||
| private static final ThreadLocal<Scratch> CURRENT = ThreadLocal.withInitial(Scratch::new); |
There was a problem hiding this comment.
This is a memory leak since the thread-local is never removed.
There is probably no way to reuse the memory permanently for a thread without adding such leak, since one cannot know if the memory will be reused in the future.
But since one Scratch can be reused for each file of a directory, that would already be a saving to reuse memory.
| MemorySegment entry = LibC.readdir(dir); | ||
| if (entry.address() == 0) { | ||
| return infos.toArray(IFileInfo[]::new); | ||
| } | ||
| // d_name is NUL terminated inside the struct, so bounding the entry by its | ||
| // declared size never cuts the name short. | ||
| MemorySegment name = dirent.d_name(entry.reinterpret(dirent.sizeof())); | ||
| String nameString = name.getString(0, PLATFORM_CHARSET); | ||
| if (!".".equals(nameString) && !"..".equals(nameString)) { //$NON-NLS-1$ //$NON-NLS-2$ | ||
| infos.add(fetchFileInfo(scratch, dirFd, name, nameString)); | ||
| } |
There was a problem hiding this comment.
Since we have at least two native calls here I'd like to understand in detail, why implementing this part with native calls is more performant than using for example Files.readAttributes(path, PosixFileAttributes.class) for each entry? Maybe even in combination with using a DirectoryStream?
I assume the information is somehow contained in #2793, but I havn't fully worked through it.
Maybe also @iloveeclipse can help, when he has time.
For example in #2952, I've also introduced a mixed approach for the Win32Handler that uses Java NIO APIs where possible and just defers to native FFM calls when it has a conceptual advantage. At least in the very simple Benchmark I did for that PR, reimplementing the DosFileAttributes read with FFM was even marginally slower. But the implementation of Win32Handler.listDirectoryAndGetFileInfos() should still be round about twice as fast since it gets the exact filename information from the DirectoryStream and doesn't have to use the slower, search based method for it.
But I assume the situation is different for POSIX since the file-system is case-sensitive.
But if something similar is possible, we could probably save even more native code here.
Furthermore when comparing this to the code in #2793, it seems to be different. E.g. here statx is used, which isn't used in the current native code.
And according to AI that's not POSIX standard and only available on Linux 4.11 onwards, but also faster.
Absolutely agree, but I'd go one step further and check where direct native calls are even necessary and where existing Java APIs can be used. Sometimes a more sophisticated use can already improve the performance. I understood your comment in #302 (comment) and your work in #2793 in that way that the main benefit of the native Linux implementation is just in the native impls of Since the native binaries now exist I think there is no great gain in having intermediate steps.
Yes, but for that we have the
For the The PosixHandler is used for macOS and Linux, so for some details they might need OS specific code-paths, but I hope the vast majority of code can be shared. And for those that I not, IMO it's fine to have separate OS specific helper classes like it's done here. And I'd say that the same mixture is fine for the |
|
I don't know what the status here is. We wait for Andreys performance tests? Or we perform performances tests ourself? @HannesWell you seem to have a clear understanding what you expect from this work, do you want to take over? I feel like I'm trying to implement something with a lot of changing parts. |
Yes, I''m fine to take over. Currently the first streamlining step of mine in #2925 needs an extension to support the BSD file-attributes used on mac. I've already started to work on that with Copilot, but got interrupted by other things. |
|
Closing as @HannesWell takes over |
On Linux,
PosixHandler.listDirectoryAndGetFileInfos()now reads a directory withopendir/readdirand stats each entry relative to it withstatxthrough the FFM API, instead of one java.nio lookup per full path.struct statxhas the same layout on every architecture, so no compiled library is needed, and the results match the java.nio path exactly.Listing 355984 entries takes 414 ms against 670 ms for java.nio and 523 ms for the JNI handler.
listDirectoryNames()keepsFile.list(), since FFM saves only about 1 µs per directory there.The bindings are generated by the included jextract script, which drops unused members; only
statxis bound by hand to captureerrno.