Assisted Inject uses child injector to bind args

[GoogleCodeExporter]

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

Original issue reported on code.google.com by dha...@gmail.com on 9 Oct 2009 at 4:07

[GoogleCodeExporter]

Yipes! Which lock?

Original comment by limpbizkit on 9 Oct 2009 at 4:30

[GoogleCodeExporter]

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

Original comment by guillegu...@gmail.com on 27 Oct 2009 at 3:25

Attachments:

• hammer_stack.txt

[GoogleCodeExporter]

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?

Original comment by m...@j.maxb.eu on 27 Oct 2009 at 3:40

[GoogleCodeExporter]

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.

Original comment by guillegu...@gmail.com on 27 Oct 2009 at 4:40

[GoogleCodeExporter]

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?

Original comment by sberlin on 27 Oct 2009 at 5:10

[GoogleCodeExporter]

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).

Original comment by crazybob...@gmail.com on 27 Oct 2009 at 5:23

[GoogleCodeExporter]

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).

Original comment by sberlin on 27 Oct 2009 at 5:30

[GoogleCodeExporter]

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?

Original comment by sberlin on 11 Mar 2010 at 5:32

[GoogleCodeExporter]

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.

Original comment by sberlin on 20 Mar 2010 at 7:55

Attachments:

• AssistedInjectPerfTest.java

[GoogleCodeExporter]

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.

Original comment by sberlin on 20 Mar 2010 at 8:54

Attachments:

• patch.txt

[GoogleCodeExporter]

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

Original comment by dha...@gmail.com on 20 Mar 2010 at 10:23

[GoogleCodeExporter]

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.

Original comment by sberlin on 20 Mar 2010 at 10:53

[GoogleCodeExporter]

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.)

Original comment by sberlin on 21 Mar 2010 at 1:39

[GoogleCodeExporter]

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.

Original comment by sberlin on 21 Mar 2010 at 3:40

Attachments:

• patch.txt

[GoogleCodeExporter]

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).

Original comment by sberlin on 22 Mar 2010 at 12:37

[GoogleCodeExporter]

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

Original comment by sberlin on 25 Mar 2010 at 3:44

[GoogleCodeExporter]

fixed in r1147.

Original comment by sberlin on 27 Mar 2010 at 8:38

• Changed state: Fixed