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

Native file info fragment for Windows on Arm64 (WoA) platform. #1351

Closed

Conversation

chirontt
Copy link
Contributor

The following new fragment for WoA is added to the project to support local file info access using native code:

org.eclipse.core.filesystem.win32.aarch64

To manually build the file info native library for WoA, on a WoA box, run the following commands:

cd resources\bundles\org.eclipse.core.filesystem\natives\win32
make.bat

and the following native library file for WoA is generated and saved in its relevant directory:

resources\bundles\org.eclipse.core.filesystem.win32.aarch64\os\win32\aarch64\localfile_1_0_0.dll

The existing 'make.bat' file mentioned above has been modified to generate correct binary for the current build environment (x64 or aarch64.)

As discussed here.

The following new fragment for WoA is added to the project to support
local file info access using native code:

org.eclipse.core.filesystem.win32.aarch64

To manually build the file info native library for WoA, on a WoA box,
run the following commands:

cd resources\bundles\org.eclipse.core.filesystem\natives\win32
make.bat

and the following native library file for WoA is generated and saved in
its relevant directory:

resources\bundles\org.eclipse.core.filesystem.win32.aarch64\os\win32\aarch64\localfile_1_0_0.dll

The existing 'make.bat' file mentioned above has been modified to
generate correct binary for the current build environment (x64 or aarch64.)
@HannesWell
Copy link
Member

HannesWell commented May 13, 2024

Thank you @chirontt for this one.
But with the numbers shared by @iloveeclipse in eclipse-platform/eclipse.platform.releng.aggregator#577 (comment) (thanks for that) I think it not worth the effort to have, build and maintain this for now. Of course assuming that the numbers are similar for Windows.
Nevertheless I think the removal of obsolete build-scripts and enhancing of the still relevant ones would be good to have.

@chirontt
Copy link
Contributor Author

I agree that @iloveeclipse 's numbers are better than any numbers I could have produced myself. So I'll drop this PR to draft, pending closing it off later if necessary.

Nevertheless I think the removal of obsolete build-scripts and enhancing of the still relevant ones would be good to have.

Not sure whether those small changes would even be needed, but I'll create a separate PR for them if needed.

@chirontt chirontt marked this pull request as draft May 14, 2024 23:47
@HannesWell
Copy link
Member

I inspected the default DosHandler in more detail and since it calls Path.toRealPath(), which is quite expensive on Windows, I fear that the numbers would be worse.

Path fileNamePath = path.toRealPath(LinkOption.NOFOLLOW_LINKS).getFileName();

As already mentioned in #1350 (comment), I think we should evaluate an JNA based implementation as an intermediate solution until we can use FFM in order to avoid more compiled native binaries.

@chirontt
Copy link
Contributor Author

I think we should evaluate an JNA based implementation as an intermediate solution until we can use FFM in order to avoid more compiled native binaries.

Yes, I agree with the JNA route; or perhaps we should start learning about FFM and skip the JNA instead.

@HannesWell
Copy link
Member

I think we should evaluate an JNA based implementation as an intermediate solution until we can use FFM in order to avoid more compiled native binaries.

Yes, I agree with the JNA route; or perhaps we should start learning about FFM and skip the JNA instead.

I would love to skip JNA and would go directly to FFM, but I assume it will take some time until the eclipse-platform requires a Java version with that we can use it. When I had some discussion about using preview-features not everybody agreed.

@HannesWell
Copy link
Member

Nevertheless I think the removal of obsolete build-scripts and enhancing of the still relevant ones would be good to have.

Not sure whether those small changes would even be needed, but I'll create a separate PR for them if needed.

I just created a PR to the clean-up all the native code and its scripts for the Windows native filesystem access:
#1416

@HannesWell
Copy link
Member

Just created #1422 to speed up the File-System access without native code.
From the numbers I obtained in a preliminary benchmark having a native fragment would not be necessary anymore.
But lets await the final results.

@HannesWell
Copy link
Member

Just created #1422 to speed up the File-System access without native code. From the numbers I obtained in a preliminary benchmark having a native fragment would not be necessary anymore. But lets await the final results.

Just run the benchmark on the working implementation and it showed that the enhanced DosHandler is on par with the native implementation: #1422 (comment)
If that is submitted I don't think we need a native implementation for Windows on ARM.

Nevertheless I think the removal of obsolete build-scripts and enhancing of the still relevant ones would be good to have.

Not sure whether those small changes would even be needed, but I'll create a separate PR for them if needed.

FYI, I did that as part of #1416.

@HannesWell
Copy link
Member

Now that #1422 is submitted, this should not be necessary anymore.
In fact I have just proposed to remove the JNI based LocalFileHandler for all supported architectures supported on Windows via #1476.

With that I think this is obsolete, but nevertheless thank you for this.

Btw. it would be nice to know if #1422 makes a noticeable difference with Windows on ARM? Can you download the upcoming I-build and report back?

@HannesWell HannesWell closed this Jul 22, 2024
@chirontt
Copy link
Contributor Author

I've used the Eclipse SDK for Windows on Arm64 from the latest I-build, but I don't notice much difference compared to other previous builds.

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

Successfully merging this pull request may close these issues.

2 participants