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

Remove usage of ObjectCleaner #8064

Merged
merged 1 commit into from Jun 28, 2018

Conversation

@normanmaurer
Copy link
Member

commented Jun 27, 2018

Motivation:

ObjectCleaner does start a Thread to handle the cleaning of resources which leaks into the users application. We should not use it in netty itself to make things more predictable.

Modifications:

  • Remove usage of ObjectCleaner and use finalize as a replacement when possible.
  • Clarify javadocs for FastThreadLocal.onRemoval(...) to ensure its clear that remove() is not guaranteed to be called when the Thread completees and so this method is not enough to guarantee cleanup for this case.

Result:

Fixes #8017.

@normanmaurer normanmaurer force-pushed the no_cleaner branch from bb5ba28 to 4d693a0 Jun 27, 2018

@normanmaurer

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2018

@jasontedor @johnou FYI as well... When working on this I also noticed that the docs not really said that onRemoval(...) is guaranteed to be called when a Thread completes.

@normanmaurer

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2018

So with this and #8062 there is no more need to use ObjectCleaner.

The behaviour of FastThreadLocal changes basically back to before this commit:
e329ca1#diff-e0eb4e9a6ea15564e4ddd076c55978de

This was introduced in 4.1.20.Final

@normanmaurer

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2018

I have also another idea to still somehow guarantee the call of onRemoval without the extra thread. I will do some experiments here and open a PR to show the idea later today.

Link head = link;
while (head != null) {
reclaimSpace(LINK_CAPACITY);
head = head.next;

This comment has been minimized.

Copy link
@johnou

johnou Jun 27, 2018

Contributor

@nitsanw should be free from GC nepotism here right?

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jun 27, 2018

Author Member

@johnou actually I think you have a good point here. We should ensure we null out next. Let me fix this

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jun 27, 2018

Author Member

@johnou PTAL again

This comment has been minimized.

Copy link
@johnou
@johnou
johnou approved these changes Jun 27, 2018
@normanmaurer

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2018

So my other idea was basically to have a ReferenceQueue and store some references in it that can handle the onRemoval when needed on a "sampling" kind of basis. While this works ( I have a working PR here) I am not sure this really is any better then just this PR alone.

@johnou

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2018

@normanmaurer I would stick with finalize until Java 9 to avoid adding unnecessary complexity unless there is a clear performance gain.

@bryce-anderson
Copy link
Contributor

left a comment

LGTM, but note that I'm light on context.

@carl-mastrangelo
Copy link
Member

left a comment

LGTM

try {
super.finalize();
} finally {
Link head = link;

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Jun 27, 2018

Member

optional: destroy link after this

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jun 27, 2018

Author Member

done

Remove usage of ObjectCleaner
Motivation:

ObjectCleaner does start a Thread to handle the cleaning of resources which leaks into the users application. We should not use it in netty itself to make things more predictable.

Modifications:

- Remove usage of ObjectCleaner and use finalize as a replacement when possible.
- Clarify javadocs for FastThreadLocal.onRemoval(...) to ensure its clear that remove() is not guaranteed to be called when the Thread completees and so this method is not enough to guarantee cleanup for this case.

Result:

Fixes #8017.

@normanmaurer normanmaurer force-pushed the no_cleaner branch from 598260c to aa0088d Jun 27, 2018

@normanmaurer normanmaurer merged commit 5b1fe61 into 4.1 Jun 28, 2018

1 check passed

continuous-integration/teamcity Finished TeamCity Build pull requests :: netty : Tests passed: 13824, ignored: 126
Details

@normanmaurer normanmaurer deleted the no_cleaner branch Jun 28, 2018

@normanmaurer normanmaurer added this to the 4.1.26.Final milestone Jun 28, 2018

@normanmaurer normanmaurer self-assigned this Jun 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.