Skip to content

Commit

Permalink
Use atomic updaters instead of atomic fields to reduce allocations fo… (
Browse files Browse the repository at this point in the history
#2462)

* Use atomic updaters instead of atomic fields to reduce allocations for commonly created objects.

* Typo
  • Loading branch information
Anuraag Agrawal authored Mar 3, 2021
1 parent dc769e3 commit 9f3dfdb
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 38 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import io.opentelemetry.context.Context;
import io.opentelemetry.context.ContextKey;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;

/**
* Helper container for Context attributes for transferring certain information between servlet
Expand All @@ -18,6 +18,14 @@ public class AppServerBridge {
private static final ContextKey<AppServerBridge> CONTEXT_KEY =
ContextKey.named("opentelemetry-servlet-app-server-bridge");

private static final AtomicIntegerFieldUpdater<AppServerBridge>
servletUpdatedServerSpanNameUpdater =
AtomicIntegerFieldUpdater.newUpdater(
AppServerBridge.class, "servletUpdatedServerSpanName");

private static final int FALSE = 0;
private static final int TRUE = 1;

/**
* Attach AppServerBridge to context.
*
Expand All @@ -42,42 +50,30 @@ public static Context init(Context ctx, boolean shouldRecordException) {
return ctx.with(AppServerBridge.CONTEXT_KEY, new AppServerBridge(shouldRecordException));
}

private final AtomicBoolean servletUpdatedServerSpanName = new AtomicBoolean(false);
private final AtomicBoolean servletShouldRecordException;
private final boolean servletShouldRecordException;

private volatile int servletUpdatedServerSpanName = FALSE;

private AppServerBridge(boolean shouldRecordException) {
servletShouldRecordException = new AtomicBoolean(shouldRecordException);
servletShouldRecordException = shouldRecordException;
}

/**
* Returns true, if servlet integration should update server span name. After server span name has
* been updated with <code>setServletUpdatedServerSpanName</code> this method will return <code>
* false</code>.
* Returns true, if servlet integration has not already updated the server span name. Subsequent
* invocations will return {@code false}. This is meant to be used in a compare-and-set fashion.
*
* @param ctx server context
* @return <code>true</code>, if the server span name should be updated by servlet integration, or
* <code>false</code> otherwise.
* @return <code>true</code>, if the server span name had not already been updated by servlet
* integration, or <code>false</code> otherwise.
*/
public static boolean shouldUpdateServerSpanName(Context ctx) {
public static boolean setUpdatedServerSpanName(Context ctx) {
AppServerBridge appServerBridge = ctx.get(AppServerBridge.CONTEXT_KEY);
if (appServerBridge != null) {
return !appServerBridge.servletUpdatedServerSpanName.get();
return servletUpdatedServerSpanNameUpdater.compareAndSet(appServerBridge, FALSE, TRUE);
}
return false;
}

/**
* Indicate that the servlet integration has updated the name for the server span.
*
* @param ctx server context
*/
public static void setServletUpdatedServerSpanName(Context ctx, boolean value) {
AppServerBridge appServerBridge = ctx.get(AppServerBridge.CONTEXT_KEY);
if (appServerBridge != null) {
appServerBridge.servletUpdatedServerSpanName.set(value);
}
}

/**
* Returns true, if servlet integration should record exception thrown during servlet invocation
* in server span. This method should return <code>false</code> on servers where exceptions thrown
Expand All @@ -91,7 +87,7 @@ public static void setServletUpdatedServerSpanName(Context ctx, boolean value) {
public static boolean shouldRecordException(Context ctx) {
AppServerBridge appServerBridge = ctx.get(AppServerBridge.CONTEXT_KEY);
if (appServerBridge != null) {
return appServerBridge.servletShouldRecordException.get();
return appServerBridge.servletShouldRecordException;
}
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,9 +164,8 @@ public static String getSpanName(HttpServletRequest request) {
* reflected in the span name.
*/
public Context runOnceUnderAppServer(Context context, HttpServletRequest request) {
if (AppServerBridge.shouldUpdateServerSpanName(context)) {
if (AppServerBridge.setUpdatedServerSpanName(context)) {
updateSpanName(Span.fromContext(context), request);
AppServerBridge.setServletUpdatedServerSpanName(context, true);
return addServletContextPath(context, request);
}
return context;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,38 +7,45 @@

import io.opentelemetry.context.Context;
import io.opentelemetry.javaagent.instrumentation.api.ContextStore;
import java.util.concurrent.atomic.AtomicReference;
import java.util.concurrent.atomic.AtomicReferenceFieldUpdater;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class State {

private static final Logger log = LoggerFactory.getLogger(State.class);

private static final AtomicReferenceFieldUpdater<State, Context> parentContextUpdater =
AtomicReferenceFieldUpdater.newUpdater(State.class, Context.class, "parentContext");

public static final ContextStore.Factory<State> FACTORY = State::new;

private final AtomicReference<Context> parentContextRef = new AtomicReference<>(null);
private volatile Context parentContext;

private State() {}

public void setParentContext(Context parentContext) {
boolean result = parentContextRef.compareAndSet(null, parentContext);
if (!result && parentContextRef.get() != parentContext) {
if (log.isDebugEnabled()) {
log.debug(
"Failed to set parent context because another parent context is already set {}: new: {}, old: {}",
this,
parentContext,
parentContextRef.get());
boolean result = parentContextUpdater.compareAndSet(this, null, parentContext);
if (!result) {
Context currentParent = parentContextUpdater.get(this);
if (currentParent != parentContext) {
if (log.isDebugEnabled()) {
log.debug(
"Failed to set parent context because another parent context is "
+ "already set {}: new: {}, old: {}",
this,
parentContext,
currentParent);
}
}
}
}

public void clearParentContext() {
parentContextRef.set(null);
parentContextUpdater.set(this, null);
}

public Context getAndResetParentContext() {
return parentContextRef.getAndSet(null);
return parentContextUpdater.getAndSet(this, null);
}
}

0 comments on commit 9f3dfdb

Please sign in to comment.