ARTEMIS-5948 Use Varhandle implementations from jctools - #6289
Conversation
|
If we could wait the 2.53.0 release.. only merge this after 2.53 please? |
|
Currently org.apache.activemq.artemis.tests.integration.karaf.ArtemisFeatureTest is failing in JDK 17 and 21 (it passes in JDK 25) on line 152. I can't really tell from the test what the underlying problem is, since the test works with a timeout it might also just be flakiness. If someone knows more about this I would appreciate any advice. |
I'd agree that would make sense. Its not yet mergeable anyhow though. |
Note that the JDK25 fast-tests job passed only because the failing test is not run on JDK25. At least at the time Artemis started working on 25, Karaf didnt work on 25, so those tests had to be omitted. I believe it failing on the line it is indicates the server bundle failed to start. I seem to recall there being a dir somewhere deep in the target dir where you can find the logs to help diagnose further. |
|
@gemmellr thanks for the advice, I had not noticed that the test isn't executed on JDK25 and I also found the logs you are referring to. Unfortunately though, they didn't tell me much. The startup fails because the server does not find I'm a bit out of my depth here and don't know what I could investigate further. Maybe you can take a look at the logs, see if anything seems off? I attached two logs to this comment, one before my changes and one after, both captured when running on JDK 17.0.16 (Temurin). |
|
From the logs the original jctools-core and new jctools-core-jdk11 modules do both seem to be getting loaded, but its failing to load an class/interface thats from the original jctools-core module, something it can only be attempting if either there is a direct usage of that still in the codebase, or if the classes used from jctools-core-jdk11 are doing it indirectly. The latter seems to be the case here, and likely gives the source of the problem. Looking at the jctools-core-jdk11 jar OSGi metadata in META-INF/MANIFEST.MF file, it does not import/say-it-uses any packages from jctools-core at all, so I expect as a result it then cant actually load the classes/interfaces that it depends upon. Essentially I'd guess you could remove |
|
@gemmellr thank you so much, that seems to have been the problem. I added the dependency in JCTools and the test now works on my machine. I already created an Issue & PR in JCTools (JCTools/JCTools#411, JCTools/JCTools#412), hopefully we can get this resolved quickly :). |
|
JCTools 4.0.7 has been released with the aforementioned fix and Artemis has been updated to use it (see #6624) so I think this can move forward. |
859fe03 to
c17b788
Compare
|
Hey @jbertram, agreed (and thanks for the heads-up). I rebased my branch now, we'll see if everything passes as expected. Are there any additional (performance) tests that you want to have done before merging? I always chose the Varhandle implementation that sounded like it was optimized for the same use-case, but the performance might of course still be different. Also FYI: the 4.0.7 release of JCTools is a little ... odd, since there is neither a github release nor a git tag for it in their repo (previous releases have both). There is a 4.x branch that looks like they could have released from there, but I'm not sure and I haven't checked the actual released contents yet. I'll open an Issue about this with JCTools directly, I just wanted to mention this here in the interest of supply-chain security. |
|
Thanks, @cortlepp! For posterity's sake, JCTools/JCTools#428 is the aforementioned issue. |
|
JCTools have tagged a release now, it seems to just have been an oversight. From my point of view this PR is ready now. |
|
@clebertsuconic or @gemmellr, do either of you have any opinions on this? This looks like a pretty straight-forward replacement. |
Makes sure that it can't be overridden by a transitive dependency (review suggenstion by @gemmellr).
I dont really know enough about this to comment on the differences, though it does seem from the 4.0.6 release notes and its mention of 5.0 going to Java 8 minimum to get VarHandles that these variants could be preferable going forward. Taking several months to release essentially a metadata fix that blocked their usage for some folks, doesnt really inspire me to be an 'early' adopter. That said, on the flip side, the change introducing it suggest its generated from the earlier mature variants, so hopefully there is minimal scope for any issues specific to the VarHandle variant. |
|
@franz1981, I know you've worked on/with JCTools. What's your opinion on these new VarHandle implementations? |
4 participants
Artemis currently uses some queues from jctools-core which rely on sun.misc.Unsafe internally. These methods are deprecated in JDK24+, issue a warning at runtime and will be removed in future versions of the JDK. Thus it makes sense to switch to an implementation which does not rely on those methods.
In their latest release JCTools have started to publish a new artifact, jctools-core-jdk11, which contains alternative Varhandle based implementations which do not use Unsafe. As far as I understand them they are designed to be a drop-in replacement to the existing Unsafe implementations.