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

remove WindowsDirectory #11772

Closed
rmuir opened this issue Sep 14, 2022 · 12 comments
Closed

remove WindowsDirectory #11772

rmuir opened this issue Sep 14, 2022 · 12 comments
Milestone

Comments

@rmuir
Copy link
Member

rmuir commented Sep 14, 2022

Description

Having native code complicates the gradle build and causes bugs.

The Direct-IO directory no longer needs native code, it uses JDK APIs. So the only thing left is WindowsDirectory.

I'm not sure this thing is really faster than the JDK. If i remember, the synchronization that causes the issues was in windows. Honestly, users should use MmapDirectory on windows IMO.

So maybe net/net it is better for our windows support to remove this thing?

@dweiss
Copy link
Contributor

dweiss commented Sep 14, 2022

I've no idea who uses this thing but I'm open to removing this - it does complicate things a bit.

@uschindler
Copy link
Contributor

+1

@rmuir
Copy link
Member Author

rmuir commented Sep 14, 2022

I kinda feel that going forwards, we would not do things this way with C code, but instead look to use the new FFI interfaces being added to openjdk if we really need to call native stuff...

@dweiss
Copy link
Contributor

dweiss commented Sep 14, 2022

If there are no objections, I'll handle the removal - there are some gradle scripts involved there that will also benefit from the cleanup.

@uschindler
Copy link
Contributor

Project Panama is the way to go, no need for FFI or anything like that. You can create a MethodHandle that downcalls into libc/kernel.dll.

I have an example in some talk.

@uschindler
Copy link
Contributor

uschindler commented Sep 14, 2022

This is how to call getpid() with Java 19 (you need to enable preview APIs and also enable access to native code (disabled for security by default):

var cLinker = Linker.nativeLinker();
// Using a MethodHandle
MethodHandle getpidMH = cLinker.downcallHandle(cLinker.defaultLookup().lookup("getpid").get(),
                    FunctionDescriptor.of(JAVA_INT));
int pid    = (int) getpidMH.invokeExact();
System.out.printf("MethodHandle calling getpid() (%d)\n", pid);

@rmuir
Copy link
Member Author

rmuir commented Sep 14, 2022

yeah, sorry when i said FFI i meant panama (its an FFI to me).

@dweiss
Copy link
Contributor

dweiss commented Sep 14, 2022

That's pretty neat.

@dweiss
Copy link
Contributor

dweiss commented Sep 15, 2022

We should apply this to main, but I think there's no harm to backport to 9.x, right? It's not a stable API and there is a safe replacement.

@rmuir
Copy link
Member Author

rmuir commented Sep 15, 2022

+1

@uschindler
Copy link
Contributor

uschindler commented Sep 15, 2022

Let's remove it. Actually the whole code is not tested at all. The removed Testcase extends LuceneTestCase and not BaseDirectoryTestcase. The only thing it does is to instantiate a Directory and an IndexOutput (!!) that is not even triggering custom code, because the native code is an IndexInput.

@dweiss
Copy link
Contributor

dweiss commented Sep 15, 2022

Applied on 9x and main.

@dweiss dweiss closed this as completed Sep 15, 2022
@rmuir rmuir added this to the 9.5.0 milestone Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants