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

[AMQ-9394] Tech Preview: Virtual Thread support #1172

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mattrpav
Copy link
Contributor

@mattrpav mattrpav commented Mar 5, 2024

Tasks:

  • Throw exception if virtualThreadTaskRunner is enabled and JDK 21 (or higher) is not available
  • Breakout the Virtual Thread factory init so it only logs on JDK 17
  • Add webpage with instructions and implementation progress status (https://activemq.apache.org/virtual-threads)
  • Update 'yield()' usage to have proper syntax
  • Add a VirtualThreadTaskRunner
  • Add activemq-client-jdk21-test module
  • Add activemq-client-jdk21 to the assembly
  • Add @experimental annotation to communicate PREVIEW status
  • Add multi-consumer, multi-queue test results

VirtualThreadTaskRunnerBrokerTest results:

[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.apache.activemq.broker.VirtualThreadTaskRunnerBrokerTest
[INFO] Tests run: 127, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 15.01 s -- in org.apache.activemq.broker.VirtualThreadTaskRunnerBrokerTest
[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 127, Failures: 0, Errors: 0, Skipped: 0

[DRAFT] Virtual Thread Phase 1 roadmap:

  1. (This PR) Provide hooks to enable Virtual Thread executor to get clock started on non-production environment testing and allow for profiling.
  2. Refactor all Thread Local references (there are only a few, non-impacting)
  3. Refactor Queue/Topic usage of synchronized -> reentrantLock
  4. Replace object monitor mutexes with Condition (not a lot of these either)

Once the above is done, that should provide the basic improvements for Virtual Thread scaling for lots of queues and topics. Once that is done, we can start the heavier refactoring to reduce blocking altogether.

[DRAFT] Virtual Thread Phase 2 roadmap:

  1. Add fast-return on queue iterate() to return when queue size is zero (no work needed)
  2. Refactor other areas that use a separate thread to just use a task or run in the queue iterate() -- expiration checking, producer flow control, etc
  3. Refactor client-side thread handling to do Future -> CompletableFutures. (I think the client-side is especially primed to take advantage of modern JDK threading features.)
  4. Update ConnectionFactory to support injecting a globally shared ExecutorService. So for high-density deployments you could have lots of connection factories using a shared Virtual Thread pool across REST service, other JMS integrations, etc.

@mattrpav mattrpav self-assigned this Mar 5, 2024
@mattrpav mattrpav changed the title [AMQ-9394] Tech Preview: Virtual Thread support WIP: [AMQ-9394] Tech Preview: Virtual Thread support Mar 5, 2024
@mattrpav mattrpav force-pushed the AMQ-9394 branch 4 times, most recently from b468134 to 121b679 Compare March 18, 2024 14:32
@mattrpav mattrpav changed the title WIP: [AMQ-9394] Tech Preview: Virtual Thread support [AMQ-9394] Tech Preview: Virtual Thread support Mar 18, 2024
@mattrpav mattrpav marked this pull request as ready for review March 18, 2024 14:33
Copy link
Contributor

@cshannon cshannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think I have to -1 merging this (into main at least) for now for a few reasons.

  1. I thought the main point was to experiment with future support (as it says tech preview) in a branch...I think it's way too early to be merging anything to do with Virtual threads into the main branch, even if it's off by default. It would be nice to see some testing done for things like performance, etc first and see what other kinds of consequences we will run into. We have no clue how things are going to go as of now with it.
  2. It makes no sense to stick with the same TaskRunner interface. The TaskRunner stuff that's in the broker we should be looking to just get rid of it anyways. It adds a lot of complexity and was added 20+ years ago before all the modern concurrent features for running tasks came about. We should be looking to get rid of that entirely and migrating to the built in Executor support already in Java and using things like Futures and CompletableFutues. So I think that should be step 1.
  3. This piggy backs onto number 2, but the VirtualThreadTaskRunner in this PR is insanely complex and I think it can go away entirely if we refactor and get rid of TaskRunner stuff.

Ultimately, I think this is fine to keep in a branch for testing and experiment for now but it's likely going to need significant changes as i pointed out above, especially because I think the TaskRunner needs to go away. A better spot for this might be to create a branch for it in ActiveMQ repo to share so it can be tested.

@cshannon
Copy link
Contributor

I should also add the caveat that while I said in my last comment I think it makes no sense to stick with the TaskRunner interface that there is still a possibility we may ultimately need to. We have a lot of custom code and impl there so we need to really dive into it to see if we can get rid of it or not or if we need to keep parts of it. There's also the possibility that we could refactor it but it's so much work it isn't worth it, but I hope not.

My initial assumption is we should be able to refactor things with modern concurrent features of java and get rid of some of that but obviously TBD. So, I don't think you should delete any of this code as maybe we end up having to use the new virtual thread task runner you created, but since that's TBD and also a tech preview that is being tested, for now I think it's best in another branch for until we figure out the plan.

I think we should take a look into the current implementations and see if they can be removed or improve/simplified as part of this. I can take a look soon.

@mattrpav
Copy link
Contributor Author

mattrpav commented Mar 22, 2024

@cshannon thanks for taking the time to review and providing the thoughtful feedback.

I agree, the threading model can be improved and leveraging more modern Java constructs would improve performance and maintainability. Overall, I think ActiveMQ is a good candidate for Virtual Threads, because the key locks are already ReentrantLocks and we have NIO on the network layer. There are a few scatter synchronized methods/blocks in queue and topic, but those can be readily refactored.

From my research of other OSS projects' (Tomcat, etc), they took a similar approach as I have proposed here -- getting Virtual Thread executor support as a configurable piece, and then use that to identify hot spots through profiling and end-user testing. From that perspective, I do feel strongly that we should work to get a configurable approach into the hands of end-users so we can get runtime hours in non-production environments. Unfortunately, I think the days of power users testing from a branch are behind us, so if we could work to some sort of compromise where its in the dist, but not guaranteed to not change I think we get the benefit of end-user testing which is really critical for this type of change.

My intent with 'Tech Preview' is to communicate that Virtual Thread support is available for testing, but not guaranteed to remain unchanged. I added the webpage to communicate that as well.

@cshannon
Copy link
Contributor

My initial comments of not to include it were because I thought you meant with Tech Preview as it's just for testing, etc and not really intended for users yet. If your intent is for users to try it out and then I think it would be ok to merge if we mark it as experimental/beta in the code itself besides just the documentation.

As I already stated, I want to look at refactoring all of the TaskRunner usage, so it's possible the final result of this looks completely different and I just didn't want to merge something in that was a preview/testing feature that might break users.

Maybe we could add something similar to @beta annotation that Guava has to mark it as a preview feature and subject to breaking changes or removal as we don't know how it will go and I don't want to add it in if we can't get rid of it later.

My guess is by the time we hit AMQ 7.0 it would certainly be considered stable but could be earlier if we did work on the threading.

I suppose the other result of this change is it requires JDK 21 to build going forward for releases which I'm not sure how I feel about. The modules are optional but obviously we need to build them if we plan to release them.

@mattrpav
Copy link
Contributor Author

My initial comments of not to include it were because I thought you meant with Tech Preview as it's just for testing, etc and not really intended for users yet. If your intent is for users to try it out and then I think it would be ok to merge if we mark it as experimental/beta in the code itself besides just the documentation.

Sounds good, I've added additional tasks here and will publish additional testing results.

Maybe we could add something similar to @beta annotation that Guava has to mark it as a preview feature and subject to breaking changes or removal as we don't know how it will go and I don't want to add it in if we can't get rid of it later.

A bit hacky, but we could use @deprecated with text that it is really beta info. Thoughts?

My guess is by the time we hit AMQ 7.0 it would certainly be considered stable but could be earlier if we did work on the threading.

Yep!

@cshannon
Copy link
Contributor

I would not use @deprecated as that means something completely different. We should probably just create our own annotation and call it @Experimental or @Beta etc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants