-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
base: main
Are you sure you want to change the base?
Conversation
b468134
to
121b679
Compare
There was a problem hiding this 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.
- 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.
- It makes no sense to stick with the same
TaskRunner
interface. TheTaskRunner
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. - 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.
I should also add the caveat that while I said in my last comment I think it makes no sense to stick with the 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. |
@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. |
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 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. |
Sounds good, I've added additional tasks here and will publish additional testing results.
A bit hacky, but we could use @deprecated with text that it is really beta info. Thoughts?
Yep! |
I would not use @deprecated as that means something completely different. We should probably just create our own annotation and call it |
Tasks:
VirtualThreadTaskRunnerBrokerTest results:
[DRAFT] Virtual Thread Phase 1 roadmap:
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: