Skip to content

Enable struct Spans with reference semantics; optimal NoopSpan #94

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 20, 2023

Conversation

ktoso
Copy link
Member

@ktoso ktoso commented Mar 16, 2023

Enable struct Spans with reference semantics; optimal NoopSpan - as per @fabianfett's request recently

Motivation:

We want a noop span to be NOT ref counted.

Modifications:

Allow spans to be value types through they must have reference semantics

Result:

Efficient noop spans


Also, Span as the protocol name again

Rationale:

since passing around any SpanProtocol a lot looked terrible;

Also, less breakage in protocol renaming
@ktoso ktoso requested review from slashmo and fabianfett March 16, 2023 11:30
@ktoso ktoso force-pushed the wip-noop-improved branch from 51927c5 to 2240874 Compare March 16, 2023 11:32
@@ -47,7 +47,7 @@ public struct NoOpTracer: LegacyTracerProtocol {
// no-op
}

public final class NoOpSpan: SpanProtocol {
public struct NoOpSpan: Span {
Copy link
Member Author

Choose a reason for hiding this comment

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

magic.

@@ -57,7 +57,7 @@ public struct NoOpTracer: LegacyTracerProtocol {
get {
"noop"
}
set {
nonmutating set {
Copy link
Member Author

Choose a reason for hiding this comment

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

"whoa whoa whoa" this saved the day.

Copy link
Member

Choose a reason for hiding this comment

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

🥳

@ktoso ktoso force-pushed the wip-noop-improved branch from 2240874 to b4d9954 Compare March 16, 2023 11:35
@stevapple
Copy link
Contributor

stevapple commented Mar 16, 2023

I believe we need some "Swift Protocol Design Guidelines" that’s going to save us from naming🤣

@ktoso ktoso force-pushed the wip-noop-improved branch from b4d9954 to b522350 Compare March 16, 2023 11:37
**Motivation:**

We want a noop span to be NOT ref counted.

**Modifications:**

Allow spans to be value types through they must have reference semantics

**Result:**

Efficient noop spans
@ktoso ktoso added this to the 1.0.0 milestone Mar 16, 2023
@ktoso ktoso force-pushed the wip-noop-improved branch from b522350 to f43657a Compare March 16, 2023 11:40
@ktoso
Copy link
Member Author

ktoso commented Mar 16, 2023

Yeah the protocol naming is rough when one uses any as well as associated types AND concrete ones would like to share the same name... but this will work okey i think

@ktoso
Copy link
Member Author

ktoso commented Mar 20, 2023

Screenshot 2023-03-20 at 11 47 02

For a dumb benchmark like this:


@_dynamicReplacement(for: registerBenchmarks) // Register benchmarks
func benchmarks() {
    Benchmark("noop spans: 1") { _ in
        _ = Tracer.withSpan("hello-1") { span in
            _ = test(span: span)
        }
    }

    Benchmark("noop spans: 3") { _ in
        _ = Tracer.withSpan("hello-1") { span in
            _ = Tracer.withSpan("hello-1-2") { span in
                test(span: span)
            }
            _ = Tracer.withSpan("hello-1-2") { span in
                test(span: span)
            }
            test(span: span)
        }
    }

    Benchmark("noop spans: 10") { _ in
        _ = Tracer.withSpan("hello-1") { span in
            test(span: span)
        }
        _ = Tracer.withSpan("hello-1") { span in
            test(span: span)
        }
        _ = Tracer.withSpan("hello-1") { span in
            test(span: span)
        }
        _ = Tracer.withSpan("hello-1") { span in
            test(span: span)
        }
        _ = Tracer.withSpan("hello-1") { span in
            test(span: span)
        }
        _ = Tracer.withSpan("hello-1") { span in
            test(span: span)
        }
        _ = Tracer.withSpan("hello-1") { span in
            test(span: span)
        }
        _ = Tracer.withSpan("hello-1") { span in
            test(span: span)
        }
        _ = Tracer.withSpan("hello-1") { span in
            test(span: span)
        }
        _ = Tracer.withSpan("hello-1") { span in
            test(span: span)
        }
    }

    let noop: NoOpTracer = NoOpTracer()
    Benchmark("concrete spans: 10") { _ in
        _ = noop.withSpan("hello-1") { span in
            test(span: span)
        }
        _ = noop.withSpan("hello-1") { span in
            test(span: span)
        }
        _ = noop.withSpan("hello-1") { span in
            test(span: span)
        }
        _ = noop.withSpan("hello-1") { span in
            test(span: span)
        }
        _ = noop.withSpan("hello-1") { span in
            test(span: span)
        }
        _ = noop.withSpan("hello-1") { span in
            test(span: span)
        }
        _ = noop.withSpan("hello-1") { span in
            test(span: span)
        }
        _ = noop.withSpan("hello-1") { span in
            test(span: span)
        }
        _ = noop.withSpan("hello-1") { span in
            test(span: span)
        }
        _ = noop.withSpan("hello-1") { span in
            test(span: span)
        }
    }
}

@inline(never)
func test(span: any SpanProtocol) -> Int {
    12
}

Would be actually fun to hook the refcount funcs as well, so we could see reduction in refcounts explicitly.

@ktoso
Copy link
Member Author

ktoso commented Mar 20, 2023

Talked with @fabianfett, I believe we're happy here 👍

@ktoso ktoso merged commit 603c916 into main Mar 20, 2023
@ktoso ktoso deleted the wip-noop-improved branch March 20, 2023 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants