-
Notifications
You must be signed in to change notification settings - Fork 195
Performance problem with defining finalizer #2045
Description
This issue asked for logging of spans that were GC:ed without being closed first.
#1876
And this PR implemented support for that by using finalizers:
#1877
I made the following simple PR to revert that behaviour, but it does not seem like it will be merged:
#2043
The reason for wanting to revert this behavior is that we saw significant performance problems that could be traced back to a large increase of finalization code in GC and also creation of opencensus spans - there is a static synchronization object that is used for that.
Avoiding finalizers is a well established practice - see Effective Java. It's also mentioned in the Checkstyle rule that was disabled: https://checkstyle.sourceforge.io/apidocs/com/puppycrawl/tools/checkstyle/checks/coding/NoFinalizerCheck.html
So instead of going directly for a PR, perhaps it would be better to start a discussion as an issue instead.
Let's try to align on a good solution before making the code changes.
An alternative solution to simply removing the finalization could be to split the class into a base class and two subclasses, where only only of the subclasses overrides finalize and then make the behaviour configurable.
Either you would want to avoid finalizers for performance reasons or you would want to enable finalizers to get logging of misuse of spans. You could even have a mix of it - use sampling to only apply finalizers to a small subset of spans. If there's a systematic problem of not closing spans it would show up anyway.
We could even take it a step further - We could start by not using finalizers but instead keep an atomic counter of currently open spans. If that is continously growing that would be an indicator that we are leaking spans and then we should enable finalizers to assist identifying the problem.