Skip to content
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

Context.Scope is great for public APIs, but may cause extra allocations for low-level things #2739

Open
bogdandrutu opened this issue Feb 6, 2021 · 2 comments
Labels
Feature Request Suggest an idea for this project release:after-ga

Comments

@bogdandrutu
Copy link
Member

bogdandrutu commented Feb 6, 2021

I think exposing Scope as the only way to attach a Context is great, but I think it would be good if the ContextStorage exposes a low-level APIs attach/detach or something similar that do not require a new object all the time.

Example:

  /**
   * Returns a {@link Runnable} that makes this the {@linkplain Context#current() current context}
   * and then invokes the input {@link Runnable}.
   */
  default Runnable wrap(Runnable runnable) {
    return () -> {
      try (Scope ignored = makeCurrent()) {
        runnable.run();
      }
    };
  }

This code needs 2 allocations (one for the return Runnable, and will do an allocation when makeCurrent is called), but we can in this case re-write this like:

  /**
   * Returns a {@link Runnable} that makes this the {@linkplain Context#current() current context}
   * and then invokes the input {@link Runnable}.
   */
  default Runnable wrap(Runnable runnable) {
    return () -> {
      Context toDetach = storage.attach(this);
      try () {
        runnable.run();
      } finally {
        storage.detach(toDetach);
      }
    };
  }

The attach/detach APIs are for example, probably we need better APIs for that and we need to think about them.

@bogdandrutu bogdandrutu added the Bug Something isn't working label Feb 6, 2021
@anuraaga
Copy link
Contributor

anuraaga commented Feb 6, 2021

I found interop with context storage implementations that use detach pattern harder since you have to implement the actual context class itself to keep track of how to restore, assuming it's not a class anyways. For example here

https://github.com/open-telemetry/opentelemetry-java/blob/main/opencensus-shim/src/main/java/io/opentelemetry/opencensusshim/OpenTelemetryCtx.java#L37

I think it's a better story for interop to use a more flexible Scope - then code can do anything it needs when restoring, not just pass in an object.

@bogdandrutu
Copy link
Member Author

@anuraaga but can we avoid that extra unnecessary allocation? Another option is to have both APIs on the Storage, so only when really needed you use the low-level once.

I actually think we can do this after 1.0 and have something like attachUnsafe and detachUnsafe as an example.

@bogdandrutu bogdandrutu added release:after-ga Feature Request Suggest an idea for this project and removed Bug Something isn't working labels Feb 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Suggest an idea for this project release:after-ga
Projects
None yet
Development

No branches or pull requests

2 participants