ARTEMIS-5972: Replace JNI with Panama Foreign Function & Memory (FFM)…#6444
ARTEMIS-5972: Replace JNI with Panama Foreign Function & Memory (FFM)…#6444mayankkunwar wants to merge 1 commit into
Conversation
|
nice one @mayankkunwar |
17ca830 to
758dfd6
Compare
a91e37f to
f177963
Compare
f177963 to
68f5845
Compare
68f5845 to
fb9503f
Compare
8ddbb55 to
8bcceee
Compare
bda5293 to
fbd575c
Compare
b602e56 to
9fc249c
Compare
beabca5 to
a89db8c
Compare
017c72c to
571ebec
Compare
| </build> | ||
|
|
||
| <profiles> | ||
| <profile> |
There was a problem hiding this comment.
Now that you made the optional compilation in the ffm module, this could always be present, right?
There was a problem hiding this comment.
I just need to link artemis-ffm changes to artemis-journal, and after that we can always have it.
| </activation> | ||
| <dependencies> | ||
| <dependency> | ||
| <groupId>org.apache.artemis</groupId> |
There was a problem hiding this comment.
this could always be present?
There was a problem hiding this comment.
yes, let me push the linking part to artemis-journal, and after this we can.
| <artifactId>maven-compiler-plugin</artifactId> | ||
| <executions> | ||
| <execution> | ||
| <id>java24-compile</id> |
There was a problem hiding this comment.
do we still need this?
There was a problem hiding this comment.
yes, we will need this as latest changes has FFM related component like MemorySegment
There was a problem hiding this comment.
what if we changed the signature to use ByteBuffer (at least what's exposed to this level?)
There was a problem hiding this comment.
actually.. never mind... the implementation of the SequentialFactory itself is dependent on other internal parts. I think we need it.
| <build> | ||
| <plugins> | ||
| <plugin> | ||
| <artifactId>maven-compiler-plugin</artifactId> |
There was a problem hiding this comment.
do we still need this?
There was a problem hiding this comment.
I guess for this case we do, but just double checking with you.
There was a problem hiding this comment.
yes, we will need this part as well
|
having the optional compilation in FFM simplifies thing a lot... nice one |
5c113de to
dd41139
Compare
| private boolean aio; | ||
|
|
||
| @Option(names = "--aio2", description = "Set the journal as asyncio 2 (Panama FFM).") | ||
| private boolean aio2; |
There was a problem hiding this comment.
I'd wonder about either something like --aio-ffm, (or maybe just reusing the existing aio option and having an additional toggle that says whether it is ffm or not (i.e jni).
'aio2' seems more like a distinct version of aio, not 'use different way to access same aio version'...and if there ever were such an aio version, it would clash.
There was a problem hiding this comment.
aio could have more variants than this, like currently we are using libaio for kernel level I/O operations and later we can replace libaio with io_uring or something else.
So the naming conventions would again change in future.
What if we mention underlying I/O libs something like: --libaio-jni / --libaio-ffm / --iouring-ffm ? or --aio-libaio-jni / --aio-libaio-ffm / --aio-iouring-ffm?
Any suggestions from this POV?
|
|
||
| if (aio) { | ||
| if (aio2) { | ||
| journalType = JournalType.ASYNCIO_2; |
There was a problem hiding this comment.
Let's decide on an agreeable naming convention.
| <version>${project.version}</version> | ||
| <scope>compile</scope> |
There was a problem hiding this comment.
Shouldnt need the version here. Implies missing entry in artemis-bom. Compile scope is default so it can be omitted.
There was a problem hiding this comment.
Good catch! Entry is missing in artemis-bom, will add an entry there and update this code
| <dependency> | ||
| <groupId>org.apache.artemis</groupId> | ||
| <artifactId>artemis-ffm</artifactId> | ||
| <version>${project.version}</version> |
There was a problem hiding this comment.
Shouldnt need this at all if the journal depends on it, since the journal will bring it in (and note the journal isnt specified here either)
There was a problem hiding this comment.
The artemis-server depends on artemis-journal, I guess we can remove this.
| } | ||
|
|
||
| public static void setForceSyscall(boolean value) { | ||
| throw new UnsupportedOperationException("Not supported for JDK < 22."); |
There was a problem hiding this comment.
Since its building to 24+, I'd make this message match?
If all the messages are the same, perhaps extract this to a constant and then update the constant.
There was a problem hiding this comment.
Will remove the unused methods.
| <dependency> | ||
| <groupId>org.apache.artemis</groupId> | ||
| <artifactId>artemis-ffm</artifactId> | ||
| <version>${project.version}</version> |
There was a problem hiding this comment.
As earlier comment, version shouldnt be needed.
| </activation> | ||
| <properties> | ||
| <its-surefire-extra-args>--add-exports java.security.jgss/sun.security.krb5=ALL-UNNAMED</its-surefire-extra-args> | ||
| <its-surefire-extra-args>--add-exports java.security.jgss/sun.security.krb5=ALL-UNNAMED |
There was a problem hiding this comment.
This seems a non-change and could be unwound.
| <dependency> | ||
| <groupId>org.apache.artemis</groupId> | ||
| <artifactId>artemis-ffm</artifactId> | ||
| <version>${project.version}</version> |
| <dependency> | ||
| <groupId>org.apache.artemis</groupId> | ||
| <artifactId>artemis-ffm</artifactId> | ||
| <version>${project.version}</version> |
| <module>artemis-jakarta-client-osgi</module> | ||
| <module>artemis-jms-server</module> | ||
| <module>artemis-jakarta-server</module> | ||
| <module>artemis-ffm</module> |
There was a problem hiding this comment.
I do wonder if this is the best module name. If its basically specific to the journal I'd wonder if it should have that in the name.
Or if the whole lot could go in the existing journal module, which is already depending on this and having its own multi-release build etc.
There was a problem hiding this comment.
Can we rename it as "artemis-aio" or something similar, or any other suggestions on this?
As we have a totally different repo for artemis-native, I thought it would be better to have these changes in separate module.
dd41139 to
7b4612e
Compare
…, which is now introducing libaio. In the configuration we are naming it as ASYNCIO_2. Co-Authored-By: Clebert Suconic <clebertsuconic@apache.org>
7b4612e to
8324ccb
Compare
… API for Journal Native Layer