Skip to content
This repository has been archived by the owner on Dec 23, 2023. It is now read-only.

Remove finalize from RecordEventsSpanImpl #2043

Merged
merged 1 commit into from Sep 22, 2020

Conversation

spkrka
Copy link
Contributor

@spkrka spkrka commented Jul 16, 2020

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

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
@@ -586,15 +586,4 @@ private RecordEventsSpanImpl(
timestampConverter != null ? timestampConverter : TimestampConverter.now(clock);
startNanoTime = clock.nowNanos();
}

@SuppressWarnings("NoFinalizer")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see original PR #1877 which fixed the issue #1876 by adding this code.
As you can see, this code change was deliberate, so removing it would probably require addressing the issue differently.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the severity of the issue this code is causing, we decided to revisit the decision and proceed with this change. See #2045 for more context.

@nilebox nilebox merged commit eab6382 into census-instrumentation:master Sep 22, 2020
zoercai pushed a commit to zoercai/opencensus-java that referenced this pull request Sep 24, 2020
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)
@spkrka spkrka deleted the krka/no_finalize branch September 24, 2020 12:15
zoercai added a commit to zoercai/opencensus-java that referenced this pull request Sep 25, 2020
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)
zoercai added a commit to zoercai/opencensus-java that referenced this pull request Sep 25, 2020
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)
zoercai added a commit that referenced this pull request Sep 25, 2020
* 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance problem with defining finalizer
3 participants