-
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 |
ARTEMIS-4937 reminded me to ask this question as I forgot, have you tried benchmarking this all or trying to verify we won't run into performance issues with pinning? I know your PR mentions refactoring but it hasn't been done and synchronized blocks are used all over the place. So I assume this will suffer from that for now, but it of course depends if it's a real issue but something we should communicate if it is. Looks like at least the issue is on it's way to being solved (hopefully for the next LTS release): https://mail.openjdk.org/pipermail/loom-dev/2024-May/006632.html |
If testing shows there are issues with performance/pinning (and I'm sure there are) we may want to wait to release this as experimental or not, it would defeat the purpose of releasing it. Or at the very least have a big warning etc. I personally don't think we should go around and just start messing with replacing a bunch of synchronized blocks just because of this. While we do need to fix the locking and synchronization in the broker, that is a major effort and needs to be well planned and is non-trival and not something that would likely happen in a 6.x release as it would be nice to modernize things for 7.x. And because there's a fix coming in the JDK (hopefully with the 25 LTS release) we could just wait for that. Other projects like Caffeine are waiting for the fix as well. |
@cshannon agree. Whether it is called “tech preview” or “experimental” is not a big deal to me. This first pass is about getting some modules so unit/itests and profiling work can begin. Having a module allows other contributors start looking at it as well. This is sufficiently complex to get going, there is no user “accidentally” turns this on in production. I will post test tool commands and results before merging any of this. As far as refactoring goes, I agree. I don’t think any wholesale search+replace of synchronized is a safe approach for stability. I was thinking about modernization of the code base, and there are plenty of extension points we could leverage to test new code paths that are more VT-friendly using new impls vs refactor-in-place. TransportConnector |
The problem here is the stated goals you just listed argue for NOT releasing this. Releases are for delivering to end users, but the goal here is not that. The goals are for profiling, testing, unit tests and for other contributors to get involved. The more I think about this the more I think it's just a really bad idea to deliver something that is completely untested and not even close to ready and likely has actual problems to end users. Instead i think a better approach is what we did on Accumulo when we had our long running "elasticity" branch. We can create a branch for this work in repo that way others can contribute and we can work on the feature until it's closer to being ready.
|
So I will say that maintaining another branch can be a bit of a pain so I think the other option I would be ok with if we want to release this now would be to just improve the documentation a lot. The feature is of course marked as a tech preview and experimental but it's not super clear as to the drawbacks or warnings if you try and use it. This page only currently lists benefits and why you would want to use it so it's kind of almost tempting an end user to want to turn it on prematurely. So I think maybe the following:
|
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: