-
Notifications
You must be signed in to change notification settings - Fork 286
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
WELD-2755 Avoid "weld-preloaders" thread group from being created repeatedly #2880
Conversation
Hello @manovotn , can you check this out? |
Hi @TomasHofman, I will take a look. And last but not least, I suppose you need this fixed for Weld 5.1.x? The |
Yes, I'm probing the territory with 6.0, eventually want to have it backported to 5.1. The app server reload is done just by executing |
There are no changes yet which would break it, but it's a preparation ground for changes coming in the next CDI (4.1).
Alright, thanks, will do. |
@manovotn I found couple of other cases that may need fixing, I will update the PR in a bit. |
40f10bd
to
9d09431
Compare
Done, I think it's ready for review. Testsuite passed locally. |
@@ -44,4 +44,9 @@ public Thread newThread(Runnable r) { | |||
thread.setDaemon(true); | |||
return thread; | |||
} | |||
|
|||
// Holder class to postpone thread group creation until when it's needed. |
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.
What's the gain here? I understand that you want a static reference to the group, but how does postponing the creation help? That thread group is referenced from AbstractExecutorServices
and as such will be created anyway during any bootstrap. What am I missing? :)
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.
The thought was that the thread group is not going to be created until someone instantiates FixedThreadPoolExecutorServices or TimingOutFixedThreadPoolExecutorServices (edit: or other AbstractExecutorServices subclass). If it's a simple static field the group is going to be created already when somebody just references the DaemonThreadFactory class, not even having to instantiate it.
There is probably not going to be a lot of difference at the end of the day, I'm OK to update this however you prefer.
private final ExecutorService executor; | ||
private final ObserverNotifier notifier; | ||
|
||
public ContainerLifecycleEventPreloader(int threadPoolSize, ObserverNotifier notifier) { | ||
this.executor = Executors.newFixedThreadPool(threadPoolSize, | ||
new DaemonThreadFactory(new ThreadGroup("weld-preloaders"), "weld-preloader-")); | ||
new DaemonThreadFactory(THREAD_GROUP, "weld-preloader-")); |
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.
I think that in this case we should just avoid using the ThreadGroup
completely. This ExecutorService
is a short-lived construct and we don't need to group the threads at all.
After discussing this with @mkouba, maybe we should drop usage of Two notable remarks to this are:
|
I'll get back to this on Mon and try to remove the usage of |
Removing the thread groups works for me too. The issue behind this PR is a "memory leak" detected by QE, where we see some class instances accumulating with every app server reload. There are not only thread groups in there, I'm just trying to look at one thing after another and see if they can be eliminated or not. If we find a problem with any of these proposed changes I'm happy to discuss and we may come to conclusion that it has to stay the way it is. After all the memory allocation related to thread groups looks to be tiny, so I don't see this as a must-have. Regarding Wildfly, I have also set of changes prepared for the Wildfly codebase (including WeldExecutorServices), but I didn't create the PR yet. If you want we can wait for Wildfly PR and then (maybe) merge this when we have some agreement there. So far we've only made a change in Infinispan (infinispan/infinispan#11277).
Aren't in general threads from various pools shared between deployments anyway (e.g. in undertow)? I assumed the weld created threads would not be deployment specific, but I deffer to your knowledge here. Thanks for all the feedback! |
I am not sure, my WFLY-fu isn't advanced enough for that :) |
@TomasHofman here are some links: Weld PR - #2881 Could you please double check that these work for you? Or rather, check that the QE tests that were used to detect it in the first place are fine with these changes. So far I don't see any issue with this on Weld side but we don't do more complex tests such as with SM - something I hope WFLY CI would show. |
Thanks @manovotn , that looks like it eliminates everything. I'm closing this PR then. |
Alright, if the WFLY side looks good as well, I will issue a similar change for Weld 5.x |
Thanks for your help @manovotn ! |
https://issues.redhat.com/browse/WELD-2755