Performance problem with defining finalizer #2045
Comments
|
Hey @spkrka, thanks for providing details about the issue you're facing due to usage of finalizer in the OpenCensus code. Given the severity of the issue you're having, and the purpose of the finalizer being purely informative (detection and logging of a common bug), I think we would agree to revert the change and merge your PR #2043. Apologies that I didn't realize the issue was causing serious performance problems for you when I initially reviewed that PR. |
|
Yes, I think #2043 would be great to merge! |
Fixes #2045 This will reduce GC pressure since the VM will not call Finalizer.register() when creating Span objects. Since this method is synchronized on a static lock it can be a source of contention. It also avoids contention on the same lock when running runFinalizer() as part of GC
|
Thanks for your patience @spkrka. The fix has been merged, we will try to make a new release within the next few days. |
|
Great, thanks! |
Fixes census-instrumentation#2045 This will reduce GC pressure since the VM will not call Finalizer.register() when creating Span objects. Since this method is synchronized on a static lock it can be a source of contention. It also avoids contention on the same lock when running runFinalizer() as part of GC (cherry picked from commit eab6382)
Fixes census-instrumentation#2045 This will reduce GC pressure since the VM will not call Finalizer.register() when creating Span objects. Since this method is synchronized on a static lock it can be a source of contention. It also avoids contention on the same lock when running runFinalizer() as part of GC (cherry picked from commit eab6382)
Fixes census-instrumentation#2045 This will reduce GC pressure since the VM will not call Finalizer.register() when creating Span objects. Since this method is synchronized on a static lock it can be a source of contention. It also avoids contention on the same lock when running runFinalizer() as part of GC (cherry picked from commit eab6382)
* Remove finalize from RecordEventsSpanImpl (#2043) Fixes #2045 This will reduce GC pressure since the VM will not call Finalizer.register() when creating Span objects. Since this method is synchronized on a static lock it can be a source of contention. It also avoids contention on the same lock when running runFinalizer() as part of GC (cherry picked from commit eab6382) * Bump version to 0.27.1 * Bump version to 0.27.2-SNAPSHOT * Bump version to 0.27.1 * Bump version to 0.27.2-SNAPSHOT
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.
The text was updated successfully, but these errors were encountered: