Cache the result of toString in BigInteger

[aarya123]

I'm currently testing out the new datadog profiling feature and am seeing a high number of MutableBigIntegers being allocated. Because it was pretty much exclusively due to toString(), I've created a simple wrapper that caches the result, which will (hopefully) reduce our overhead of logging the same trace id multiple times.

[devinsba]

Hi, thanks for the contribution!

Did you consider instead or creating a new type, adding the caching to DDSpanContext directly?

Here:

dd-trace-java/dd-trace-ot/src/main/java/datadog/opentracing/DDSpanContext.java

Line 136 in fc6c327

public String toTraceId() {

And here:

dd-trace-java/dd-trace-ot/src/main/java/datadog/opentracing/DDSpanContext.java

Line 149 in fc6c327

public String toSpanId() {

[aarya123]

Unfortunately, I don't think that would be a thorough enough of a fix. At the end of the day, the problems may not lie with the users of the tracing api library, but the implementors of integrations as well! There are many places where a span is cast to a DDSpan and then call getTraceId(), which isn't inherently wrong since in some contexts, since you do need the BigInteger for things like sampling. But there are quiet a few places that use the toString() method on that object, be it implicit or explicit.
The type that the TraceId and SpanId uses is an implementation detail and it seems more encompassing to me to make the change there as opposed to the interfaces that OpenTracing itself makes available to the users in order to protect contributors and users of this agent.

[tylerbenson]

Caused by: org.gradle.api.GradleException: Rule violated for class datadog.opentracing.StringCachingBigInteger: instructions covered ratio is 0.4, but expected minimum is 0.6

Looks like jacoco has issue with the new class's test coverage. You can add it to the ignore list here: https://github.com/DataDog/dd-trace-java/blob/master/dd-trace-ot/dd-trace-ot.gradle#L13

[tylerbenson]

Technically this isn't threadsafe, but in practice I don't think it matters.

[dougqh]

Yes, in this case, it will work, since the calculation is idempotent.
Each thread will eventually make the null -> non-null transition.
This is the same basic strategy employed by String for the hashCode calculation.

The downside is a potential for a bit of extra allocation compared to the "ideal", but that's a reasonable trade-off.
This will already save a great deal on allocation and keeping the coordination overhead down is also important.

It could be made volatile, but we'd still have the same fundamental race -- so I think this is good as is.

[aarya123]

keeping the coordination overhead down is also important.

Super agreed on this point. The overhead in a massively parallel/concurrent environment would not be worth locking this value