-
15. Re: Does ScopeKey need to maintain a sorted (in ScopeLevel.l
jaikiran Aug 10, 2009 2:10 AM (in response to smarlow)Not to sidetrack this discussion, but now that we are refactoring this class, it would be good to have some meaningful javadocs on this one so that the next time we try to fix something in this, we won't have to guess how this class is expected to behave.
-
16. Re: Does ScopeKey need to maintain a sorted (in ScopeLevel.l
smarlow Aug 10, 2009 5:07 PM (in response to smarlow)"alesj" wrote:
"smarlow@redhat.com" wrote:
I would like to make a deeper pass of changing some of the calls into ScopeKey. This work will be across different projects (aop and others).
What kind of changes are we talking about here?
Or why is the change to an array not good enough?
I changed to an array (built at constructor time). I would like to evaluate what the performance improvement of this change (and a few other small ones) is. As part of this experiment, I want to assess the gain from changing the callers to pass the Scopes at construction time (if this works, that will help avoid object lock contention during parallel deployment). -
17. Re: Does ScopeKey need to maintain a sorted (in ScopeLevel.l
smarlow Aug 11, 2009 8:12 AM (in response to smarlow)Ales,
This can probably be simplified more but I need to test these changes first. I also started to add Javadoc comments. The comment at the top are still rough but gives a little more information. However, this is not the time to review my changes, I need to measure the results first. Of course, feel free to take anything out of here and include in your local ScopeKey.
The link to review the current changes is http://pastebin.com/m516c0bdf
I would like to complete this task soon as this has been taking longer than I wanted and I need to work on other tasks. I would like to integrate this change with the other project branches, but I need to know which branches should be used (preferably the ones that the change would be checked into, which I assume is trunk but wanted to confirm).
I agree that there is risk to changing the projects to accommodate this change but I think we should evaluate that risk after finishing the code changes (but before code is checked in). That way, we know what we are gaining and what we are losing.
Scott -
18. Re: Does ScopeKey need to maintain a sorted (in ScopeLevel.l
adrian.brock Aug 11, 2009 9:39 AM (in response to smarlow)I don't understand why you have to change the ScopeKey?
I do understand that making it immutable with a builder is a better design, but
you're likely to break things if you change the semantics of the api.
THE PROBLEM:
If you go back to Jason's comments he talks about using a builder to create
a ScopeKey, but that's all a scope key really is.
The brain dead part is that it gets used at runtime in the BasicMetaDataRepository
as the key to a map which it then freezes to avoid mutablity.
The issue being that you still could incur contention because
of the synchronization on the scopes within the ScopeKey traversed in equals()/hashCode().
SOLUTIONS:
There are at least two ways you can fix this without changing the external api.
(The suggestions on improving the internal implementation should still be done)
1) Rather than having the synchronization and freeze() you could just
clone() the ScopeKey so that nobody can modify the ScopeKey once it is used by
the MDR (it is a different object), making the synchronizations irrelevant
but getting the same semantics.
2) The other is as I mention above. Use the ScopeKey as the builder class
and retrieve a more optimizated key for internal use within the MDR.
e.g. something like:public class ScopeKey { ... Object getOptimzedKey() {} // Key for use at runtime }
In fact (1) and (2) are just variations on the same theme, with (2) potentially
being more optimal.
POST SCRIPT
One further optimization would be to cache the hashCode()
since the key is used in ConcurrentHashMap. But I don't know that it is called
enough times (outside bootstrap/configuration) to make that worthwhile?
But, you're obviously seeing something to have looked at this in the first place? -
19. Re: Does ScopeKey need to maintain a sorted (in ScopeLevel.l
jaikiran Aug 11, 2009 9:39 AM (in response to smarlow)I would like to integrate this change with the other project branches
If we have a list of the projects affected (need changes) then i guess it would be easier to know which branch/trunk to work on. -
20. Re: Does ScopeKey need to maintain a sorted (in ScopeLevel.l
smarlow Aug 11, 2009 9:58 AM (in response to smarlow)"adrian@jboss.org" wrote:
The brain dead part is that it gets used at runtime in the BasicMetaDataRepository
as the key to a map which it then freezes to avoid mutablity.
The issue being that you still could incur contention because
of the synchronization on the scopes within the ScopeKey traversed in equals()/hashCode().
I assume you mean in the current implementation (even with freeze, synchronization is needed). With the proposed changes, we don't need the synchronization on the scopes within the (immutable) ScopeKey. -
21. Re: Does ScopeKey need to maintain a sorted (in ScopeLevel.l
adrian.brock Aug 11, 2009 10:46 AM (in response to smarlow)"smarlow@redhat.com" wrote:
"adrian@jboss.org" wrote:
The brain dead part is that it gets used at runtime in the BasicMetaDataRepository
as the key to a map which it then freezes to avoid mutablity.
The issue being that you still could incur contention because
of the synchronization on the scopes within the ScopeKey traversed in equals()/hashCode().
I assume you mean in the current implementation (even with freeze, synchronization is needed). With the proposed changes, we don't need the synchronization on the scopes within the (immutable) ScopeKey.
BACKWARDS COMPATIBILITY
I'm saying immutability breaks backwards compatibility (in our code and in other
code which might not even have been written yet but could get written against the
existing api in the released jboss5/mc binaries).
That's because the ScopeKey is used as a builder.
Our main use case is just to build a APPLICATION/DEPLOYMENT scope
from the "Default Scope" and an INSTANCE/CLASS scope on top a DEPLOYMENT.
This is done with a clone() and addScope().
So if you clone it or use some more optimized object in the MDR itself then this
synchronization is not required.
REASON FOR SCOPEKEY BEING MUTABLE
You do need something like the ScopeKey being mutable until the last minute
because things like the CLASS scope may not be known until the bean instance
is constructed from a factory. Only then do you know the class.
There was also an additional (unimplemented or at least unused) requirement
to allow services to add extra scopes. e.g. an EJB could add a CLUSTER scope to get
shared clustered metadata.
Or similarly allowing a SUBSYSTEM scope for shared defaults, e.g. SUBSYSTEM=EJB
So each stage of the deployment can modify the full MDR scope until it gets registered.
YOUR SOLUTION VS MY SOLUTION
Changing all the other api, e.g. DeploymentUnit, KernelScopeInfo, etc.
to make this "mutable scope holder" some other class would be a lot more effort
(besides breaking existing - or even still to be written - use cases).
It would be better to just the leave the ScopeKey as the builder and optimize the
runtime in one of the ways I suggested (or maybe some other way you can think of?). -
22. Re: Does ScopeKey need to maintain a sorted (in ScopeLevel.l
adrian.brock Aug 11, 2009 11:10 AM (in response to smarlow)"adrian@jboss.org" wrote:
You do need something like the ScopeKey being mutable until the last minute
because things like the CLASS scope may not be known until the bean instance
is constructed from a factory. Only then do you know the class.
By the way, that's another HACK (and marked as such in the code)
see KernelScopeInfo and InstantiateAction.
If you can think of a better way to do it then feel free to fix it. ;-) -
23. Re: Does ScopeKey need to maintain a sorted (in ScopeLevel.l
adrian.brock Aug 11, 2009 11:19 AM (in response to smarlow)By the way, ScopeKey is Serializable so don't break the serialized form if you change the internal state.
Although the only place I can think of where this could "leak" onto a different
machine is via the Deployment jmx management
see org.jboss.deployers.structure.spi.DeploymentBean -
24. Re: Does ScopeKey need to maintain a sorted (in ScopeLevel.l
jason.greene Aug 11, 2009 3:43 PM (in response to smarlow)Yeah, I agree with Adrian, this approach is better than my suggestion since its less work and we can likely merge it into the 5.x series.
-
25. Re: Does ScopeKey need to maintain a sorted (in ScopeLevel.l
smarlow Aug 12, 2009 10:16 AM (in response to smarlow)"adrian@jboss.org" wrote:
2) The other is as I mention above. Use the ScopeKey as the builder class
and retrieve a more optimizated key for internal use within the MDR.
e.g. something like:public class ScopeKey { ... Object getOptimzedKey() {} // Key for use at runtime }
In fact (1) and (2) are just variations on the same theme, with (2) potentially
being more optimal.
I'm trying to do (2) as follows.public class ScopeKey { ... /** * The returned ScopeKey is immutable and optimized for use at runtime. * @return Optimized immutable ScopeKey */ public ScopeKey getOptimizedKey() { return new UnmodifiableScopeKey(this); } }
Where UnmodifiableScopeKey is similar to the code that I previously posted.
There is a dependency between UnmodifiableScopeKey + ScopeKey that needs to be manually maintained but maybe I could test for that in a UnmodifiableScopeKey unit test. UnmodifiableScopeKey extends ScopeKey and overides all of the public ScopeKey methods.
The (work in progress) UnmodifiableScopeKey source is here http://pastebin.com/m5d484e25
Does UnmodifiableScopeKey work for everyone? I assume this will work as the optimizedKey. -
26. Re: Does ScopeKey need to maintain a sorted (in ScopeLevel.l
smarlow Aug 18, 2009 12:25 PM (in response to smarlow)Updates to ScopeKey are here http://pastebin.com/m2fc19bab
UnmodifiableScopeKey is here http://pastebin.com/m19feda54
Perhaps we could make further MDR changes to reduce the number of times that ScopeKey.equals is called during AS startup. The above changes shouldn't break any existing API and we still pass the serialization ScopeKey unit test. If we agree on making these changes, I'll add unit tests for UnmodifiableScopeKey.
Other changes:
Index: src/main/java/org/jboss/metadata/plugins/repository/basic/BasicMetaDataRepository.java
===================================================================
--- src/main/java/org/jboss/metadata/plugins/repository/basic/BasicMetaDataRepository.java (revision 92394)
+++ src/main/java/org/jboss/metadata/plugins/repository/basic/BasicMetaDataRepository.java (working copy)
@@ -108,8 +108,9 @@
{
if (retrieval == null)
throw new IllegalArgumentException("Null retrieval");
- ScopeKey key = retrieval.getScope();
+ ScopeKey key = retrieval.getScope().getOptimizedKey();
key.freeze();
return retrievals.put(key, retrieval);
}
Index: src/main/java/org/jboss/metadata/plugins/context/AbstractMetaDataContext.java
===================================================================
--- src/main/java/org/jboss/metadata/plugins/context/AbstractMetaDataContext.java (revision 92394)
+++ src/main/java/org/jboss/metadata/plugins/context/AbstractMetaDataContext.java (working copy)
@@ -114,7 +114,8 @@
for (Scope scope : scopes)
key.addScope(scope);
}
- scopeKey = key;
+ scopeKey = key.getOptimizedKey();
}
return scopeKey;
} -
27. Re: Does ScopeKey need to maintain a sorted (in ScopeLevel.l
smarlow Aug 18, 2009 1:39 PM (in response to smarlow)"adrian@jboss.org" wrote:
"adrian@jboss.org" wrote:
You do need something like the ScopeKey being mutable until the last minute
because things like the CLASS scope may not be known until the bean instance
is constructed from a factory. Only then do you know the class.
By the way, that's another HACK (and marked as such in the code)
see KernelScopeInfo and InstantiateAction.
If you can think of a better way to do it then feel free to fix it. ;-)
Would it be better to use an UnmodifiableScopeKey instance to represent the frozen state? ScopeKey.getOptimizedKey() currently (in my local repository) returns an UnmodifiableScopeKey. -
28. Re: Does ScopeKey need to maintain a sorted (in ScopeLevel.l
smarlow Aug 19, 2009 10:46 AM (in response to smarlow)I'm using j.u.c.ConcurrentSkipListMap which depends on Java 6.
As an alternative, we could use the JBoss port (JBCOMMON-83) of ConcurrentSkipListMap.
I'll add some unit tests for the ScopeKey changes. -
29. Re: Does ScopeKey need to maintain a sorted (in ScopeLevel.l
smarlow Aug 21, 2009 11:52 PM (in response to smarlow)Unit tests for ScopeKey.getOptimizedKey() are here http://pastebin.com/m4742f057
I added a little more javadoc to ScopeKey which is here http://pastebin.com/m4e6ab9ad
We still need to discuss the use of Java 6 (will help us scale better and Java 5 is hitting end of life October 30, 2009).