-
Notifications
You must be signed in to change notification settings - Fork 112
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
Migrate the Windows native file-system refresh monitor to JNA #1394
Migrate the Windows native file-system refresh monitor to JNA #1394
Conversation
...lipse.core.resources/src/org/eclipse/core/internal/resources/refresh/win32/Win32Natives.java
Outdated
Show resolved
Hide resolved
resources/bundles/org.eclipse.core.resources.win32.x86_64/META-INF/MANIFEST.MF
Show resolved
Hide resolved
1f92e90
to
8c7c99c
Compare
Thanks for asking. We have disabled refresh monitor in all our workspaces by default, so that i do not need to care about this particular feature. In general i think that replacing native code with java code would eclipse easier to maintain. But com.sun.jna is not standard java. However jna is already used in eclipse already anyway.
i.e. it is slower? 10% may be some seconds on large workspaces. I don not get the benefit of this change - it's not standard java, it is slower. So it has cons. Where are the pros that outweigh them? aarch64 seems to be a insufficient argument because almost nobody uses it. |
Wouldn't it be better to wait for java 22 Foreign Function support? |
FYI, Java 25 LTS is scheduled for September 2025, so following the current practice of upgrading to a new Java LTS release with about 3-6 months after GA, that has us waiting until into 2026... |
2026 doesn't sound bad - or is there any need to rush optimizations for the 0% (rounded) user of the IDE that use aarch64? |
I would consider JNA as a ordinary third-party dependency and I don't that it's usage would in general be discouraged? Of course there are alternatives with other pro's and cons (JNI - faster but more cumbersome, FFM - not yet available).
Relatively the composite benchmark case is 10% with JNA compared to JNI, but if you look at the absolute numbers you can see that the difference is about 1.5µs. This means the three actions, adding a watcher, obtaining a change notification and closing the watcher have to run more than 600.000 times (roughly rounded to smaller numbers) to sum up to one second delay. I assume nobody will ever face that scenario, but if it would be possible I such operations would take so much longer that the one second would hardly be measurable in the other noise and would not be noticed by a human at all.
This is not an optimization for aarch64 on Windows, it implements a feature that is currently not supported with that architecture in a way that also tries to reduce the maintenance effort in general and the effort to support the new architectures (probably there are non soon). Two birds with one stone. |
8c7c99c
to
12518a6
Compare
How should we proceed here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not submit anything that is known to be slower without a need. Either proof with a reproducible measurement that the regression is insignificant, i.e. < 0.1% in a real world use case of a workspace with 10_000 folders and a million files that are updated or do not touch the windows specific code but add another codepath for arm. Just submitting something that is known to be slower and leaving it to others to find the regression is inappropriate. Relying on guesses is not enough.
Can you please explain where this number is coming from? Was there some kind of agreement about it or is this number general practice? Furthermore was the reference this relation is applied to? If there is such a specific number I would expect a specific scenario to test.
Not sure if a million files are really a real world scenario. I would say the entire Eclipse SDK with all its four sub-projects and even more git sub-modules is pretty large and it only has 115.435 files and 30.264 folders (if the Windows Explorer didn't count it wrong). A project almost nine time the size of it will probably have some challenges. If I understood your suggestion for a benchmark correctly, you suggest to have one project with 10_000 folders where a millions files are added? Therefore I'm not sure that you have read my analysis in #1394 (comment) in detail? Just looking at the numbers is not sufficient. Without context and analysis every benchmark is worthless and it can be wrong in any direction. Monitors are just added to project root folders and linked resources: Therefore to really challenge the new implementation one would have to create many projects that are empty and that all get one file added. Adding more files to the same project would just increase the overall runtime while there is still only one call to the native methods and thus its ratio would be even smaller. RefreshPerformanceTest code
For
and the runtime with the new one based on JNA:
One immediately notices that creating and awaiting the changes takes much longer than all other parts and much longer than expected only from the operations. When looking at Line 557 in 309a7f0
it is clear that the processing of changes found by native hooks is throttled after the current changes in all monitor directories are processed. Since RESCHEDULE_DELAY has a value of 3000 the ten projects match the runtime of that part of 30,000s quite well and I think one can assume that the throttling happens after each change in a project in this case.
Consequently there is no point in aiming for the last µs in the implementation that awaits the change notifications. But even with the throttling of changes active we can still benchmark the impact of the slower JNA implementation on installing and removing the listener on project opening and closing. For that I simply commented out the middle part that creates the files in the project and waits a change notification. Since the case now runs much faster I changed the number of process projects to 1000.
Of course the runtime of such benchmark cases varies from run to run and one has to run them multiple times to get smaller Confidence intervals (e.g. I don't expect this change to generally speed up deleting projects by ~15%). Nevertheless this helps to put the numbers of the JMH-benchmark I presented initially into perspective. Bear in mind that the native method But to put this into perspective: Even if With that I think it is clear that, as expected, that this change is less than an insignificant in terms of performance. |
12518a6
to
b119182
Compare
First of all please read the second paragraph in #1394 (comment). In your benchmark you basically just measure the runtime of refreshing a project that contains large amount of files and folders. The change detection on OS level that involves the code changed with this PR is only called once, maybe a few times more depending on how long the script touching files runs.
That's because you still have the
Throttling happens only between the calls that ask the OS if there are new changes within the entire project directory structure. Because you create only one big change in one project the OS is only queried once for changes within the project directory (as written above maybe a few times more depending on how long it takes to touch all files). Everything else you see is the runtime of refreshing the entire project. Just like if you had trigger a refresh manually. |
Maybe something is being smart and figures out to require every available (and applicable via the filters) fragment for a given host. |
The o.e.core.resources.win32.x86_64 fragment is removed because the Windows native file-system refresh monitor is migrated to use JNA instead of native code (JNI). See eclipse-platform/eclipse.platform#1394
ece6602
to
0866cec
Compare
This avoids the native code necessary for JNI and consequently the separate o.e.core.resources.win32.x86_64 fragment, while being only slightly slower without noticeable impact on the overall performance. At the same time it allows to use the 'Win32RefreshProvider' on all CPU architectures supported on Windows out of the box. Also part of eclipse-platform/eclipse.platform.releng.aggregator#577
0866cec
to
9daf733
Compare
The o.e.core.resources.win32.x86_64 fragment is removed because the Windows native file-system refresh monitor is migrated to use JNA instead of native code (JNI). See eclipse-platform/eclipse.platform#1394
Great. I made a few final clean-ups and checks and will submit it as soon as the build is green.
Yes these references are removed via eclipse-platform/eclipse.platform.releng.aggregator#2127. |
The o.e.core.resources.win32.x86_64 fragment is removed because the Windows native file-system refresh monitor is migrated to use JNA instead of native code (JNI). See eclipse-platform/eclipse.platform#1394
This avoids the native code necessary for JNI, while being only slightly slower. At the same time it allows to use the 'Win32RefreshProvider' on all CPU architectures on Windows.
This is an alternative to #1350, avoiding the need to recompile the native binaries for Windows on aarch64.
Part of eclipse-platform/eclipse.platform.releng.aggregator#577
In order to benchmark this I created the following JMH benchmark that measures the runtime of a simple sequence of method calls that creates a monitor handle, asks for changes and then disposes the handle again.
Testing the methods in isolation would be more difficult, but if one has suggestions I can benchmark other scenarios as well.
`Win32MonitorBenchmark` source code
The
Win32Natives
replicates the existing JNI based implementation of theWin32Natives
and uses the existingdll
, the classWin32NativesNew
replicates the new JNA based implementation.The results are
In these scenarios the JNA based implementation is overall less then 10% slower.
From the four remaining relevant native methods
long FindFirstChangeNotificationW(WString lpPathName, int bWatchSubtree, int dwNotifyFilter)
int FindCloseChangeNotification(long hChangeHandle)
int FindNextChangeNotification(long hChangeHandle)
int WaitForMultipleObjects(int nCount, long[] lpHandles, int bWaitAll, int dwMilliseconds)
the method
FindNextChangeNotification
is the only one that could be performance sensitive.FindFirstChangeNotificationW
andFindCloseChangeNotification
are only called once if a project is opened respectively closed. And one cannot wait faster for events user initiatesWaitForMultipleObjects
is also not very sensitive.But looking at the absolute numbers, I think in practice other factors will have a much larger impact and a 10% slow down at such low level will hardly be measured in real-life applications or will be noticed.
With that I think the performance changes due to this change are acceptable.
@jukzi since you are often interested in improving the performance, what do you think?
@merks since you are also working a lot on Windows, maybe this is also interesting for you.
Of course everyone else is also invited to discuss this.