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
Ensure cancelled scheduled tasks can be GC'ed ASAP #3994
Conversation
@trustin please check |
Is there a reason you kept the purge task as a no-op? Isn't it OK to remove it completely? |
@trustin let me see if I can remove it. Rest looks ok? |
Yeah, I didn't know DelayedQueue doesn't have the culprit of PriorityQueue. |
@trustin what you mean with culprit? |
Having to remove the cancelled tasks periodically? But.. perhaps we didn't have to do a periodic removal even with PriorityQueue - e.g we could keep the list of cancelled tasks in a mpsc queue and then remove them from PriorityQueue in the event loop. I guess DelayedQueue vs. PriorityQueue + MPSC queue might worth a comparison/benchmark. WDYT? |
Or we could just add a new cancel task to the eventloop if cancel is called outside of the EventLoop. This would maybe create a few more objects but may be good enough. @trustin WDYT? |
SGTM |
@trustin alright, will update PR later today |
0596179
to
b6c7f44
Compare
@trustin please check again! I kept the NOOP task as this way we still have the "quit period" of 1 second before the thread is shutdown. WDYT ? |
b6c7f44
to
01df80e
Compare
Ah OK. How about adding some comment that explains what it does? We could also rename NOOP to something more contextually meaningful. |
Will do... Any good name you can think of? Ready to cherry-pick after it?
|
QUIET_PERIOD_TASK? |
Yeah, OK to cherry-pick |
Sounds good!
|
Motivation: Prior we used a purge task that would remove previous canceled scheduled tasks from the internal queue. This could introduce some delay and so use a lot of memory even if the task itself is already canceled. Modifications: Schedule removal of task from queue via EventLoop if cancel operation is not done in the EventLoop Thread or just remove directly if the Thread that cancels the scheduled task is in the EventLoop. Result: Faster possibility to GC a canceled ScheduledFutureTask.
01df80e
to
bc100c9
Compare
Motivation:
Prior we used a purge task that would remove previous canceled scheduled tasks from the internal queue. This could introduce some delay and so use a lot of memory even if the task itself is already canceled.
Modifications:
Use a DelayedQueue to store the ScheduledFutureTasks and remove the tasks directly once it was canceled sucessfully.
Result:
Faster possibility to GC a canceled ScheduledFutureTask.