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

Add a memory-mapped RandomAccessReader using MemorySegment api #296

Merged
merged 2 commits into from
Apr 17, 2024

Conversation

mdogan
Copy link
Collaborator

@mdogan mdogan commented Apr 16, 2024

I'd prefer this to be in jvector-twenty module. But --enable-preview flag is only allowed for the Java release version used to compile the code. When building with Java 22, --enable-preview is not allowed on twenty module because it builds for Java 20.

Closes #278

@mdogan mdogan force-pushed the memory-segment-reader branch from 9ffd328 to edf50bb Compare April 16, 2024 14:58
@jbellis
Copy link
Owner

jbellis commented Apr 16, 2024

Outstanding!

Can you wire this into ReaderSupplierFactory (replacing MMapReaderSupplier)? Not sure if reflection is needed given the module configuration.

@jkni
Copy link
Collaborator

jkni commented Apr 16, 2024

Thank you @mdogan! I feel your pain here regarding module placement. We could adopt Maven Toolchains so that we could call out to a Java 20 compiler in jvector-twenty, allowing us to use preview features there. Any strong feelings about whether that's worth it? It only introduces marginal additional complexity in the build.

mdogan added 2 commits April 17, 2024 10:30
I'd prefer this to be in `jvector-twenty` module. But `--enable-preview` flag is only
allowed for the Java release version used to compile the code. When building with Java 22,
`--enable-preview` is not allowed on `twenty` module because it builds for Java 20.
@mdogan mdogan force-pushed the memory-segment-reader branch from edf50bb to ee2fb37 Compare April 17, 2024 08:19
@mdogan
Copy link
Collaborator Author

mdogan commented Apr 17, 2024

Outstanding!

Can you wire this into ReaderSupplierFactory (replacing MMapReaderSupplier)? Not sure if reflection is needed given the module configuration.

Done.

@mdogan
Copy link
Collaborator Author

mdogan commented Apr 17, 2024

Thank you @mdogan! I feel your pain here regarding module placement. We could adopt Maven Toolchains so that we could call out to a Java 20 compiler in jvector-twenty, allowing us to use preview features there. Any strong feelings about whether that's worth it? It only introduces marginal additional complexity in the build.

Actually I don't care about Java 20 but maybe it would be useful for Java 21 users (since it's LTS). But your new native extensions are targeting 22, so this can remain in Java 22 too.

@jbellis
Copy link
Owner

jbellis commented Apr 17, 2024

LGTM. Ship it!

@jbellis
Copy link
Owner

jbellis commented Apr 17, 2024

(pushing the button since i want to put this in beta3)

@jbellis jbellis merged commit e287eb0 into jbellis:main Apr 17, 2024
6 checks passed
@mdogan mdogan deleted the memory-segment-reader branch April 18, 2024 06:12
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.

add a RandomAccessReader implementation for jdk 20+ using modern MMap or MemorySegment
3 participants