Dynamic casting

[isaachier]

I am concerned about the requirement to use dynamic_cast to downcast from an opentracing span to a Jaeger span for my Jaeger client implementation. It is a potential performance bottleneck I'd like to avoid. Is it possible to use the visitor pattern instead? For example, instead of

    class Tracer : public opentracing::Tracer {
        // ...
        virtual opentracing::expected<void> Inject(
            const opentracing::SpanContext& sp,
            const opentracing::TextMapWriter& writer) const override
        {
            // Forced to use dynamic_cast here to downcast to Jaeger span
            const SpanContext* ctx = dynamic_cast<const SpanContext*>(&sp);
            if (!ctx) {
                return opentracing::make_expected_from_error<void>(
                    opentracing::invalid_span_context_error);
            }
            // Encode Jaeger context...
            return opentracing::make_expected();
        }

can SpanContext have a virtual method named Inject with a default implementation that returns an error so I could write something like this?

    class SpanContext : public opentracing::SpanContext {
        // ...
        opentracing::expected<void> Inject(const opentracing::TextMapWriter& writer) const override
        {
            // Encode Jaeger context...
            return opentracing::make_expected();
        }
        // ...
    };

    class Tracer : public opentracing::Tracer {
        // ...
        virtual opentracing::expected<void> Inject(const opentracing::SpanContext& sp,
                                                                                const opentracing::TextMapWriter& writer) const override
        {
            return sp.Inject(writer);
        }
    };

In fact, that latter would probably remove the need for a Tracer::Inject method entirely.

[yurishkuro]

The inject method is defined on the Tracer API for parity with the extract method, which has to be on the Tracer. Remember that OT API's main objective is to be convenient for the end user, not for the implementor. Is there a performance hit from doing a dynamic cast?

[isaachier]

The is a minor performance hit. Mostly, taboo in the C++ community (see https://google.github.io/styleguide/cppguide.html#Run-Time_Type_Information__RTTI_ and http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-dynamic_cast). I'm pretty certain the C++ tracing team at Bloomberg rejected opentracing-cpp almost entirely on the basis of its use of dynamic_cast.

[rnburn]

@isaachier The issue of dynamic_cast was discussed a lot in the PRs from Bloomberg (#6 and #7). I don't believe consensus was ever reached on a design that avoided it.

[isaachier]

I think that was really more about templates vs. virtual functions. Maybe I missed it, but there does not seem to be any objection to the visitor method approach.

[rnburn]

@isaachier -- one issue I can think of for an Inject design like you propose is what happens if you mix spans from different tracers. For example, if I do something like

auto spanA = tracerA->StartSpan("ASpan");

// Now I switch the tracer implementation to be tracerB

tracerB->Inject(spanA->context(), carrier); 

// If this calls spanA->context().Inject(carrier), it will inject the span using tracerA's
// format and I won't get any error message. But if tracerB were to do a dynamic_cast 
// first then I'd get back an `invalid_span_context_error`.

However, I'm not sure that's such a bad thing.

I think you'd still want to have the Inject method on the tracer to handle the case of the CustomCarrier, but maybe it's ok to add an Inject method on the SpanContext that can be optionally called by the tracer. Let me do a little more research on this.

[isaachier]

OK I see that. Although I'll say it is definitely a rare scenario. This is known as the multiple dispatch problem, and in that case, I'd be OK sticking with dynamic_cast. There is no good solution I know of for that.

[pantoss]

I would be very interested to see a benchmark of the two approaches? With high optimisation level I suspect this is very little difference to the point that it might be the other way around to the way you expect. The cost of the virtual function call might be more than the dynamic cast check as you would loose out on any possibility of the compiler inlining and branch prediction might also play a significant role.

To make it really faster you could always overload the Inject method on your tracer so that if the caller knows that the SpanContext is the appropriate type no checking or virtual calls need to be made.

class Tracer : public opentracing::Tracer {
       // ...
       virtual opentracing::expected<void> Inject(
           const opentracing::SpanContext& sp,
           const opentracing::TextMapWriter& writer) const override
       {
           // Forced to use dynamic_cast here to downcast to Jaeger span
           const SpanContext* ctx = dynamic_cast<const SpanContext*>(&sp);
           if (!ctx) {
               return opentracing::make_expected_from_error<void>(
                   opentracing::invalid_span_context_error);
           }
           return Inject(sp, writer);  // This might get inlined.
       }

     // If the caller has a SpanContext instead of a opentracing::SpanContext this inject overload will
    // be called. It might even get inlined if your are using LTO
      opentracing::expected<void> Inject(
           const SpanContext& sp,
           const opentracing::TextMapWriter& writer) const
   {
      // Encode Jaeger context...
      return opentracing::make_expected();
   }

[isaachier]

I'm not against trying both. I think I read in one of the Effective C++ books by Scott Meyers that (at least in the old days of C++98) there is no exact specification for how an implementation must perform `dynamic_cast`. That means it could vary from comparing integers internally to comparing class names as strings. That was the main concern with `dynamic_cast`. For similar reasons, I've heard `typeid` can be much faster because it need not traverse the virtual hierarchy.

…

On Oct 17, 2017 3:14 AM, "pantoss" ***@***.***> wrote: I would be very interested to see a benchmark of the two approaches? With high optimisation level I suspect this is very little difference to the point that it might be the other way around to the way you expect. The cost of the virtual function call might be more than the dynamic cast check as you would loose out on any possibility of the compiler inlining and branch prediction might also play a significant role. To make it really faster you could always overload the Inject method on your tracer so that if the caller knows that the SpanContext is the appropriate type no checking or virtual calls need to be made. `class Tracer : public opentracing::Tracer { // ... virtual opentracing::expected Inject( const opentracing::SpanContext& sp, const opentracing::TextMapWriter& writer) const override { // Forced to use dynamic_cast here to downcast to Jaeger span const SpanContext* ctx = dynamic_cast<const SpanContext*>(&sp); if (!ctx) { return opentracing::make_expected_from_error( opentracing::invalid_span_context_error); } return Inject(sp, writer); // This might get inlined. } // If the caller has a SpanContext instead of a opentracing::SpanContext this inject overload will // be called. It might even get inlined if your are using LTO opentracing::expected<void> Inject( const SpanContext& sp, const opentracing::TextMapWriter& writer) const { // Encode Jaeger context... return opentracing::make_expected(); } ` — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACdXeWPPAJqa-O-mY7GD78UntlD6R-13ks5stFPAgaJpZM4P6z0W> .

[isaachier]

Will close for now until I have proof that this affects performance.