@Provides providing a Provider?

[Sam Berlin]

Hi Folks,

It's currently not possible to have an @Provides Provider<Foo> method in a Module (Guice throws an exception saying binding to Provider is not allowed).  While technically this is achievable by having a class implement Provider & binding to the class, I find the shorthand of @Provides to be much much easier & simpler (with parameters passed into the method as dependencies, etc..).  Would it be possible to relax that restriction and allow a binding to a Provider act the same as bind(..).toProvider(...)?

Thanks!

Sam

[Robbie Vanbrabant]

Doesn't @Provides work by wrapping those method invocations with a Provider instance? If you do @Provides Foo you will already be able to inject a provider for Foo. Not sure if that is what you mean.

Robbie

[Sam Berlin]

Ya, it already does that, which is nice.  But, there's some cases where we want the Provider to have a bit more complexity to it, and binding to those providers requires a bit of annoying code right now.  It'd be nice to be able to reuse the simplified @Provides syntax and return those Providers directly.  Essentially it'd be a reverse Provider method.  Instead of Guice creating a Provider implementation that calls a method, Guice would be binding to the Provider implementation supplied by the method.

For example, we have a large settings framework where each setting implements Provider<T>.  I'd like to be able to bind those methods with a simple:  @Provides @MyBindingAnnotation Foo methodName() { return myFooSetting; }, rather than bind(Foo.class).annotatedWith(MyBindingAnnotation.class).toProvider(myFooSetting);.   In this example, it's just syntactic sugar, but when the Provider requires dependencies, it can reduce a whole boilerplate of code.

Sam

[limpb...@gmail.com]

Why not just call 'get()' on the returned provider at the end of the
method?

Ie. go from this:
@Provides Provider<Foo> provideFooProvider() {
Provider<Foo> provider = ...
return provider;
}

...to this:
@Provides Foo provideFoo() {
Provider<Foo> provider = ...
return provider.get();
}

[Sam Berlin]

That's one way of doing it.  I suppose it'd work, too, so long as provideFoo wasn't marked as a singleton (because we do want Foo to change through its lifetime).  But, it has the sort-of-bad behavior of effectively creating two Providers for Foo, the ProviderMethod provider & the real Provider.

Sam

[limpb...@gmail.com]

On Sep 15, 11:51 am, Sam Berlin <sber...@gmail.com> wrote:
> That's one way of doing it.  I suppose it'd work, too, so long as provideFoo
> wasn't marked as a singleton (because we do want Foo to change through its
> lifetime).  But, it has the sort-of-bad behavior of effectively creating two
> Providers for Foo, the ProviderMethod provider & the real Provider.

One thing to keep in mind - whenever you bind a Provider<Foo> and then
inject a Provider<Foo>, you get different provider instances. To
illustrate:
final FooProvider a = new MyFooProvider();
Injector injector = Guice.createInjector(new AbstractModule() {
public void configure() {
bind(Foo.class).toProvider(a);
}
});
Provider<Foo> b = injector.getProvider(Foo.class);
assertFalse(a == b);
assertFalse(b instanceof FooProvider);

With scopes, the difference is even more pronounced. If we were to
scope the Foo binding in singleton scope, repeated calls to a.get()
would still return different instances.

[Sam Berlin]

Ha!  That makes perfect sense, and completely obviates the need for what I was asking for.  So, blah -- good job making a consistent no-frills API. :-)

On a somewhat related, but mostly different note, in profiling & performance tuning we've seen a lot of overhead from Provider.get calls.  This is usually related to a lot of creation of throwaway Errors objects.  I'm not sure what, if anything, can be done about this.  In the case of AssistedInject there's even worse overhead by trying to bind objects to the parent injector that will always fail & then be created in the child injector.  In those instances, I think an additional InjectorBuilder.setJitBindingsEnabled(false) API would do wonders (and also solve a lot of conceptual difficulties people have had with child injectors adding bindings to the parent).

Sam

[limpb...@gmail.com]

On Sep 15, 7:08 pm, Sam Berlin <sber...@gmail.com> wrote:
> On a somewhat related, but mostly different note, in profiling & performance
> tuning we've seen a lot of overhead from Provider.get calls.  This is
> usually related to a lot of creation of throwaway Errors objects.  I'm not
> sure what, if anything, can be done about this.

Hmmm...

Would you be interested in seeing what happens if you pool the Errors
objects? You'd need to go through the code and find hot calls to "new
Errors()" with something to pull them from a cache, plus something
else to return unused ones to the cache. If you try this and get
sizable performance improvements, I'll accept a patch!

> In the case of AssistedInject there's even worse overhead by trying to bind objects to the
> parent injector that will always fail & then be created in the child
> injector.  In those instances, I think an additional
> InjectorBuilder.setJitBindingsEnabled(false) API would do wonders (and also
> solve a lot of conceptual difficulties people have had with child injectors
> adding bindings to the parent).

Yeah, AssistedInject is currently a bit of a mess internally. Sooner
or later I'd like to change it from using child injectors to use the
new 'toConstructor()' API introduced after Guice 2.0. Since nobody
else is actively working on this at the moment, patches are welcome
here.

By the way, I'm sorry that I still haven't done anything about the old
patches you've posted to our bugtracker. It's tricky business trying
to balance the API between exposing enough power, and exposing too
much! I'm still thinking things over, and in general I'd like to be a
Good Open Source Project and accept good patches.

Thanks again buddy,
Jesse

[Dhanji R. Prasanna]

On Wed, Sep 16, 2009 at 3:59 PM, je...@swank.ca <limpb...@gmail.com> wrote:

Yeah, AssistedInject is currently a bit of a mess internally. Sooner
or later I'd like to change it from using child injectors to use the
new 'toConstructor()' API introduced after Guice 2.0. Since nobody
else is actively working on this at the moment, patches are welcome
here.

toConstructor() is the most badass API that nobody uses. =D

 

[Sam Berlin]

Hmmm...

Would you be interested in seeing what happens if you pool the Errors
objects? You'd need to go through the code and find hot calls to "new
Errors()" with something to pull them from a cache, plus something
else to return unused ones to the cache. If you try this and get
sizable performance improvements, I'll accept a patch!

I'll take a look.  It'll likely require some sort of ThreadLocal & Stack, something similar to other performance optimizations you've already done.

Yeah, AssistedInject is currently a bit of a mess internally. Sooner
or later I'd like to change it from using child injectors to use the
new 'toConstructor()' API introduced after Guice 2.0. Since nobody
else is actively working on this at the moment, patches are welcome
here.

Hmmm... I'm not certain I fully understand.  I see how toConstructor can match up to an arbitrary constructor (ie, that'd let you use @AssistedInject again instead of @Inject), but I don't quite see how it could remove the need for a child injector.  All the examples from the tests seem to require the other parameters to the constructor as bindings in the Module.  I'd love to be able to clean this up, so any pointers are welcome.
 

By the way, I'm sorry that I still haven't done anything about the old
patches you've posted to our bugtracker. It's tricky business trying
to balance the API between exposing enough power, and exposing too
much! I'm still thinking things over, and in general I'd like to be a
Good Open Source Project and accept good patches.

No worries -- I know exactly how it is with patches piling up... there's a lot around here that I just haven't had a chance to look at.  We patch when necessary, but for the most part the important things (most recently getAllBindings) make it in.

Sam