Assisted Inject uses child injector to bind args

[gissuebot]

From dhanji on October 09, 2009 00:07:04

This causes MASSIVE lock contention if used inside a running app. Which is Bad(TM)

Original issue: http://code.google.com/p/google-guice/issues/detail?id=435

[gissuebot]

From limpbizkit on October 08, 2009 21:30:37

Yipes! Which lock?

[gissuebot]

From guilleguti84 on October 27, 2009 08:25:49

I want to bump this, doing Assisted Injection in a multithreaded application makes
all threads but one to block. Please look at the attached thread dump for detailed
stack trace.

Regards,

Guillermo

Attachment: gist
   hammer_stack.txt

[gissuebot]

From [email protected] on October 27, 2009 08:40:52

Lock contention is not the only problem with use of a child injector for assisted
inject. Creating a new injector for each injection is a rather costly process itself,
which doesn't really seem necessary?

[gissuebot]

From guilleguti84 on October 27, 2009 09:40:26

A quick update. We had a bottleneck in an Assisted Inject, and we were able to remove
the bottleneck and made it like 200 times faster by replacing the
FactoryProvider.newFactory(Whatever.class) inside the Module with a bind to our own
implementation of that factory (that just get the instances of the dependencies and
call the constructor).

By changing that the time inside the Inject was so small that the lock was almost
unnoticeable.

[gissuebot]

From sberlin on October 27, 2009 10:10:28

the reason for using a parent/child injector is mainly to provide AOP, right?  would
a possible approach be to figure out if AOP applies to any of the methods & then
fallback to not using child injectors (and instead doing it the 'old assisted inject
way'.. with Proxys)?  are there reasons for using parent/child injectors other than AOP?

[gissuebot]

From crazyboblee on October 27, 2009 10:23:44

The correct solution here is to fix createChildInjector() to not lock. It's locking so as to
ensure it doesn't install a binding that's also in the parent, but we can certainly do this
in a concurrent fashion (using methods on ConcurrentHashMap, for example).

[gissuebot]

From sberlin on October 27, 2009 10:30:21

when issue 342 is fixed (disable JIT bindings), child injectors whose parent injector
disables JIT bindings won't need to lock (or use concurrent lookups either).

[gissuebot]

From sberlin on March 11, 2010 09:32:42

A lot of the performance penalties are coming from creating a new child injector each
time the Factory is used.  Is it absolutely necessary to create a new child injector?
 It's being done right now so that different args can be passed to different
invocations.  Could this also be done by using ThreadLocals & Providers?  What if all
the factory-provided arguments were provided by internal mutable ThreadLocal
Providers, and each invocation set the ThreadLocal so that the pre-created child
injector's Providers would return it?

[gissuebot]

From sberlin on March 20, 2010 12:55:05

Attached is a pretty quick & dirty TestCase for to illustrate the perf differences
between old assistedinject & new assistedinject (and for good measure, new assisted
inject + requiring explicit bindings).  After a 1000 iteration warmup of each case,
the time required to do 100,000 factory creates is:
 old style assisted inject: ~800ms
 new style assisted inject: ~15,000ms
 new style assisted inject + require explicit bindings: ~13,000ms

I'll play around with some perf tools to see just where the problems are (it isn't
lock contention here, 'cause this is a single threaded test), as well as experiment
with changing to using toConstructor bindings explicitly.  If anyone has done perf
analysis, please post where you've found the problems to be.

Attachment: gist
   AssistedInjectPerfTest.java

[gissuebot]

From sberlin on March 20, 2010 13:54:33

Using toConstructor does help things a lot, but it's still significantly slower.
Changing FactoryProvider2 to use toConstructor bindings has this runtime (same setup
as the prior comment) --

old style assisted inject: ~800ms
new style assisted inject: ~11,000ms
new style assisted inject + require explicit bindings: ~11,000ms

So it shaved off ~4s in 100k runs, which is pretty significant, but is still ~10s
slower than the old assisted inject.

(Yes, using pure time values is relative on all machines, but these values are all
from my particular home machine, so comparing them to each other is OK.)

Attached is the proof-of-concept patch I did in FactoryProvider2 to make it use
toConstructor.

Attachment: gist
   patch.txt

[gissuebot]

From dhanji on March 20, 2010 15:23:45

Very Cool! Btw a few comments, from my experience microbenching:

- were these just single runs or did you average them over several? IME, they seem to vary a lot

• I notice your warmup has 1000 runs, even in client mode that's not enough to catch the JIT thresholds =(
• if you ran on a mac this will look very different to linux
• I think the major problem in this code is the lock contention, so even a penalty of 300ms (from old assisted)
  should be acceptable if we are able to remove that

[gissuebot]

From sberlin on March 20, 2010 15:53:11

I didn't average the runs, but I did do it several times and eyeballed the runs to be
within ~500ms of each other.   I ran these particular tests on Windows XP... I'll
give it a go on Linux on Monday (and will look for a Mac box to test it on). Lock
contention should mostly be fixed by using requireExplicitBindings (I think),
because the child injector doesn't have to look in the parent injector to potentially
create JIT bindings anymore.

Changing the warmup to be 100k runs and repeating both warmup & test 10 times for
each case doesn't seem to significantly change the numbers for me.  FactoryProvider1
averages on the higher end of ~750ms, and using toConstructor-patched version of
FactoryProvider2 (with or without explicit bindings required) averages on the higher
end of ~10s.

[gissuebot]

From sberlin on March 20, 2010 18:39:49

Some stats on profiling with YourKit tracing (w/ the toConstructor patch, since
that's an obvious win already, and could be used in some way/shape/form to enable
multi-constructor bindings at some point)...

1k runs w/ a FactoryProvider2's factory.create, with profiling enabled takes ~4s.
 - 3.5s: InternalInjectorCreator.build()
   - 2.5s: InjectorShell$Builder.build()
     - 1.5s: Elements.getElements(Stage, Iterable)
       - 1.2s: Elements$RecordingBinder.install(Module)
         - 1s: AbstractModule.configure(Binder)
           - 1s: FactoryProvider2$2.configure()
             - ~700ms: BindingBuilder.toConstructor(Constructor, TypeLiteral)
               - ~600ms: InjectionPoint.forConstructor(Constructor, TypeLiteral)
                 - ~600ms: InjectionPoint.<init>(TypeLiteral, Constructor)
                   - ~400ms: Constructor.getParameterAnnotations()
                   - ~200ms: InjectionPoint.forMember(Member, TypeLiteral,
Annotation[][])
             - ~200ms: BindingBuilder.toProvider(Provider)
         - ~200ms: Elements$RecordingBinder.install(Module)
       - ~300ms: Elements$RecordingBinder.<init>(Stage)
     - ~400ms: AbstractProcessor.process(InjectorImpl, List)
     - ~200ms: MembersInjectorStore.<init>(InjectorImpl, List)
     - ~200ms: InjectorImpl.<init>(InjectorImpl, State,
InternalInjectorCreator$InjectorOptions)
   - ~500ms: InternalInjectorCreator.initializeStatically()
   - ~300ms: InternalInjectorCreator.injectDynamically()

That's the path of the worst offenders, mostly everything over ~100ms, traced down
within the worst path.

There's nothing clearly heinous about it except for the fact that creating a child
injector is a big operation and takes time.  (For comparison, the same profiling
setup recorded the old style assistedinject as taking 1s total for 1000 runs.)

[gissuebot]

From sberlin on March 21, 2010 08:40:50

Since profiling didn't reveal any obvious places to optimize (just the entire flow of
creating a child injector is bulky and takes a while), I toyed around with a
different way of optimizing: Cache the child-injector/binding and use a ThreadLocal
for the Provider of the arguments.  This optimization only works (and is only used)
if none of the assisted injection points inject Provider itself and if Injector also
isn't injected.  With the optimization, the a 1k warmup & 100k test looks like this:

 old style assisted inject: ~800ms
 new style optimized assisted inject: ~150ms
 new style optimized assisted inject + require explicit bindings: ~150ms

Clearly a big win as far as performance goes.  Can anyone think of any reason why the
optimization wouldn't be safe if it isn't used when Providers or Injector are injected?

Patch to apply optimization to FactoryProvider2 is attached.

Attachment: gist
   patch.txt

[gissuebot]

From sberlin on March 21, 2010 17:37:30

I should note, the optimization patch in comment #14 also fixed the the lock
contention problems (because child injectors are not (re)created for each factory call).

I was also mistaken about requireExplicitBindings fixing the lock contention -- the
lock contention was from InheritingState.lock() sharing the lock of its parent's
state and is still used when creating the child injectors.  Fixing
createChildInjector to not lock (as Bob suggests in comment #6) is a good idea, but I
think the patch in comment #14 is still a good thing to do anyway, because it
bypasses the massive overhead of creating a child injector frequently (leading to a
~6500% speedup).

[gissuebot]

From sberlin on March 24, 2010 20:44:53

committed r1147 (includes tests) which should fix this problem for 99.9% of
use-cases.  it will not fix it if the assisted object injects Injector or a Provider
of an assisted key.  i appreciate any and all code reviews!

i think this can pave the way for:
 1) remove logic for the "old style assisted inject".
 2) solve issue 430 by undeprecating & reusing @ AssistedInject to mark valid
construcotrs.
 3) deprecate FactoryProvider in favor of FactoryModuleBuilder

[gissuebot]

From sberlin on March 27, 2010 13:38:30

fixed in r1147 .

Status: Fixed