Sni/lock free #45
Sni/lock free #45
Conversation
stephanenicolas
force-pushed
the
sni/lock-free
branch
from
ad7e0ee
to
287b4ec
Compare
|
Coverage decreased (-10.5%) to 75.115% when pulling 74a2fda5baefed77802f3d35575d5a2e4e26264e on sni/lock-free into c46f936 on master. |
stephanenicolas
force-pushed
the
sni/lock-free
branch
from
74a2fda
to
287b4ec
Compare
| } | ||
| removeScopeAndChildrenFromMap(scope); | ||
|
|
||
| public static synchronized void closeScope(Object name) { |
There was a problem hiding this comment.
do you still need synchronized here? or only synchronized when scope != null
There was a problem hiding this comment.
why openScope() is not synchronized and closeScope() is?
|
Coverage decreased (-10.9%) to 74.669% when pulling 8784b2fbbaea03be11f503754062251374015b99 on sni/lock-free into c46f936 on master. |
| MAP_KEY_TO_SCOPE.put(name, scope); | ||
| } | ||
| Scope scope = MAP_KEY_TO_SCOPE.get(name); | ||
| if (scope != null) { |
There was a problem hiding this comment.
We have:
... A
../ ...
B.....C
And then:
- Thread 1 calls -> closeScope(A)
- Thread 2 calls -> openScope(C)
Thread 2 can get scope C when it is about to be removed. Should we allow that?
There was a problem hiding this comment.
yes, t2 should get something. The idea of the lock free structures is that when a thread reads and another thread writes, the reader sees the data structure either before or after the change, but not during the transition. And I believe it's the case here. In a multi-threaded context, you should be careful that your scope was not closed before using it.
Closing a scope will detach it from the tree, and C will become a root of a new tree indeed. With synchronized, the exact same problem will happen a millisecond later if some thread closes the scope you just opened.
My point of view is that this can be more or less adressed by DI itself. In Android for instance, the main thread starts a background threads with a callback, when the main thread leaves the context, it should remove the callback and interrupt the background thread or not. If the background thread uses only the application level scope, it can be allowed to continue will the activity is dead, but if it uses activity scope or lower, it should be stopped.
So in one case, you can use the scope, in the other not. Really, I don't think this can be fully solved by DI. What I favored here is performance.
stephanenicolas
force-pushed
the
sni/lock-free
branch
from
75ece4a
to
3162741
Compare
|
Coverage decreased (-0.4%) to 84.873% when pulling d8d7c1861f7199c720ef68d44d919b091ee73fc8 on sni/lock-free into 3f70ec5 on master. |
|
Coverage decreased (-0.4%) to 84.81% when pulling 2f5684dfb34a68a16f327c89ed10ec21ef3b991b on sni/lock-free into 3f70ec5 on master. |
|
Only coveralls fails (-0.4%) but build passes. |
| public synchronized InternalProviderImpl<? extends T> getProvider(String bindingName) { | ||
| InternalProviderImpl<? extends T> internalProvider = mapNameToProvider.get(bindingName); | ||
| return internalProvider; | ||
| public InternalProviderImpl<? extends T> getProvider(String bindingName) { |
There was a problem hiding this comment.
your implementation may not crash since it only write once. But it may crash or return unexpected value when the hashmap is resizing
http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/util/HashMap.java#390
http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/util/HashMap.java#HashMap.resize%28int%29
stephanenicolas
force-pushed
the
sni/lock-free
branch
from
48e8a9f
to
5ba7feb
Compare
DO NOT MERGE
Besides Daniel, I would love Michael & Henri to review this.
The goal of this PR is to
This version provides an improvement for massive DI :
For 55 K injections.