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

Ensure cancelled scheduled tasks can be GC'ed ASAP #3994

Closed
wants to merge 1 commit into from

Conversation

normanmaurer
Copy link
Member

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.

@normanmaurer normanmaurer self-assigned this Jul 16, 2015
@normanmaurer normanmaurer added this to the 4.0.30.Final milestone Jul 16, 2015
@normanmaurer
Copy link
Member Author

@trustin please check

@trustin
Copy link
Member

trustin commented Jul 18, 2015

Is there a reason you kept the purge task as a no-op? Isn't it OK to remove it completely?

@normanmaurer
Copy link
Member Author

@trustin let me see if I can remove it. Rest looks ok?

@trustin
Copy link
Member

trustin commented Jul 18, 2015

Yeah, I didn't know DelayedQueue doesn't have the culprit of PriorityQueue.

@normanmaurer
Copy link
Member Author

@trustin what you mean with culprit?

@trustin
Copy link
Member

trustin commented Jul 18, 2015

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?

@normanmaurer
Copy link
Member Author

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?

@trustin
Copy link
Member

trustin commented Jul 18, 2015

SGTM

@normanmaurer
Copy link
Member Author

@trustin alright, will update PR later today

@normanmaurer
Copy link
Member Author

@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 ?

@trustin
Copy link
Member

trustin commented Jul 19, 2015

Ah OK. How about adding some comment that explains what it does? We could also rename NOOP to something more contextually meaningful.

@normanmaurer
Copy link
Member Author

Will do... Any good name you can think of? Ready to cherry-pick after it?

Am 19.07.2015 um 02:02 schrieb Trustin Lee notifications@github.com:

Ah OK. How about adding some comment that explains what it does? We could also rename NOOP to something more contextually meaningful.


Reply to this email directly or view it on GitHub.

@trustin
Copy link
Member

trustin commented Jul 19, 2015

QUIET_PERIOD_TASK?

@trustin
Copy link
Member

trustin commented Jul 19, 2015

Yeah, OK to cherry-pick

@normanmaurer
Copy link
Member Author

Sounds good!

Am 19.07.2015 um 06:46 schrieb Trustin Lee notifications@github.com:

QUIET_PERIOD_TASK?


Reply to this email directly or view it on GitHub.

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.
@normanmaurer
Copy link
Member Author

Cherry-picked into 4.0 (36c80cd) , 4.1 (05dae57) and master (d64168e)

@normanmaurer normanmaurer deleted the cancel_scheduled_tasks branch July 19, 2015 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants