See these two bugs for background:
https://glassfish.dev.java.net/issues/show_bug.cgi?id=1727
https://glassfish.dev.java.net/issues/show_bug.cgi?id=1800
Changes:
1. Use of 'synchronized' in a few places to force visibility on
AMXImplBase variables.
2. Use of 'final' in several places so as to avoid the need for
'synchronized' or volatile. In one case, the method
getInterfaceMBeanInfo() was introduced, so that the variable could be
assigned once, thus making it possible to be 'final'.
3. Use of 'volatile' for mQueryMgr to force thread-safety (visibility).
4. Optimization of highly-inefficient method getAttributeClass().
Now the classes are stored in a Map of Maps (one Map per j2eeType).
The Map maps the j2eeType to a Map of <String,Class> which is used in
getAttributeClass(). This cuts down considerably on the overhead of
finding an Attribute's class. See bug #1800 for details.
Thanks,
LLoyd
Timeout: 2pm, 12/18.
----------------
MB2:/gf/build/glassfish/admin/mbeanapi-impl lloyd$ cvs diff -u -w src/
java/com/sun/enterprise/management/support/AMXImplBase.java
Index: src/java/com/sun/enterprise/management/support/AMXImplBase.java
===================================================================
RCS file: /cvs/glassfish/admin/mbeanapi-impl/src/java/com/sun/
enterprise/management/support/AMXImplBase.java,v
retrieving revision 1.21
diff -u -w -r1.21 AMXImplBase.java
--- src/java/com/sun/enterprise/management/support/
AMXImplBase.java 2 Dec 2006 04:02:10 -0000 1.21
+++ src/java/com/sun/enterprise/management/support/
AMXImplBase.java 19 Dec 2006 02:06:16 -0000
@@ -140,15 +140,16 @@
/**
The interface this MBean implements
*/
- private Class mInterface;
+ private final Class mInterface;
/**
The MBeanInfo
*/
- private MBeanInfo mMBeanInterfaceMBeanInfo;
+ private final MBeanInfo mMBeanInterfaceMBeanInfo;
/**
The parent MBean for this MBean.
+ No need to make volatile; a recompute is OK if one thread
doesn't yet see it.
*/
private ObjectName mCachedContainerObjectName;
@@ -159,17 +160,16 @@
private final boolean
mEmitAttributeChangeNotifications;
- private QueryMgr mQueryMgr;
- private AMX mSelfProxy;
+ private volatile QueryMgr mQueryMgr;
+ private volatile AMX mSelfProxy;
private ConnectionSource mConnectionSource;
/**
An optional Delegate
*/
- private Delegate mSuppliedDelegate;
- private Delegate mDelegate;
-
+ private final Delegate mSuppliedDelegate;
+ private volatile Delegate mDelegate;
private AMXAttributeNameMapper mAttributeNameMapper = null;
@@ -180,6 +180,11 @@
protected CoverageInfo mCoverage;
+ /**
+ Maps a j2eeType to a Map<String,Class> which maps an
Attribute name to a Class.
+ */
+ private static final Map<String,Map<String,Class>>
ATTRIBUTE_CLASSES =
+ Collections.synchronizedMap( new
HashMap<String,Map<String,Class>>() );
protected Class
getInterface( final String j2eeType )
@@ -274,6 +279,24 @@
return wasEnabled;
}
+ private MBeanInfo
+ getInterfaceMBeanInfo(
+ final Class theInterface )
+ {
+ MBeanInfo info =
+ MBeanInfoConverter.getInstance().convert
( theInterface, getExtraAttributeInfos());
+
+ if ( getAMXDebug() )
+ {
+ debug( "Interface " + mInterface.getName() +
+ " has MBeanInfo:\n" +
+ MBeanInfoStringifier.DEFAULT.stringify
( info ) );
+
+ info = addDebugMBeanInfo( info );
+ }
+ return info;
+ }
+
/**
Construct a new implementation that implements the
supplied mbeanInterface.
@@ -328,6 +351,9 @@
((DelegateBase)mSuppliedDelegate).setDebugOutput
( getDebugOutput() );
}
+ ATTRIBUTE_CLASSES.put( mJ2EEType,
+ Collections.synchronizedMap( new HashMap<String,Class>
() ) );
+
// initialization of mDelegate is deferred until
later; the supplied delegate
// may not be in a position to run yet
mDelegate = null;
@@ -336,17 +362,7 @@
mAttributeInfos = null;
mFullType = null;
- mMBeanInterfaceMBeanInfo =
- MBeanInfoConverter.getInstance().convert
( mInterface, getExtraAttributeInfos());
-
- if ( getAMXDebug() )
- {
- debug( "Interface " + mInterface.getName() +
- " has MBeanInfo:\n" +
- MBeanInfoStringifier.DEFAULT.stringify
( mMBeanInterfaceMBeanInfo ) );
-
- mMBeanInterfaceMBeanInfo = addDebugMBeanInfo
( mMBeanInterfaceMBeanInfo );
- }
+ mMBeanInterfaceMBeanInfo =
getInterfaceMBeanInfo( mInterface );
}
private static MBeanAttributeInfo[]
EXTRA_ATTRIBUTE_INFOS = null;
@@ -547,7 +563,7 @@
}
- public synchronized Delegate
+ public Delegate
getDelegate()
{
return( mDelegate );
@@ -792,22 +808,33 @@
return( result );
}
+ /**
+ Called every time an Attribute is obtained via
delegateGetAttribute(), so
+ make sure it's reasonably fast.
+ */
private Class<?>
getAttributeClass( final String attributeName )
throws ClassNotFoundException
{
- final MBeanAttributeInfo[] infos = getMBeanInfo
().getAttributes();
+ final Map<String,Class> mappings = ATTRIBUTE_CLASSES.get
( getJ2EEType() );
- Class theClass = null;
+ Class theClass = mappings.get( attributeName );
+ // initialize only if the class is null and there isn't a
mapping for to null
+ if ( theClass == null && ! mappings.containsKey
( attributeName ) )
+ {
+ // no need to synchronize; the Map is already so.
+ // And if mappings were somehow 'put' twice, that's rare
and of no importance
+ final MBeanAttributeInfo[] infos = getMBeanInfo
().getAttributes();
- // do it the dumb way; later we can optimize
+ // Map each Attribute to a Class
for( int i = 0; i < infos.length; ++i )
{
- if ( infos[ i ].getName().equals
( attributeName ) )
- {
- theClass =
ClassUtil.getClassFromName( infos[ i ].getType() );
- break;
+ final String attrName = infos[ i ].getName();
+ final Class c = ClassUtil.getClassFromName( infos
[ i ].getType() );
+ mappings.put( attrName, c );
}
+
+ theClass = mappings.get( attributeName );
}
return( theClass );
@@ -856,6 +883,9 @@
getMBeanLogger().warning
( "AMXImplBase.delegateGetAttribute: " +
"Can't find class for
attribute: " + name + "=" + value +
" in object " + getObjectName
() );
+
+ // add a null mapping
+ ATTRIBUTE_CLASSES.get( getJ2EEType() ).put( name,
null );
}
}
@@ -928,11 +958,8 @@
{
if ( mAttributeInfos == null || !
getMBeanInfoIsInvariant() )
{
- synchronized( this )
- {
mAttributeInfos =
JMXUtil.attributeInfosToMap( getMBeanInfo().getAttributes() );
}
- }
return( mAttributeInfos );
}
@@ -2183,7 +2210,12 @@
return( nameOut );
}
- public final ObjectName
+ /*
+ Note that this method is 'synchronized'--to force visibility
of all fields it affects.
+ Since it's called only once (per instance) for an MBean
Registration, it has no performance
+ impact on later use, but guarantees visibility of all non-
final instance variables.
+ */
+ public final synchronized ObjectName
preRegister(
final MBeanServer server,
final ObjectName nameIn)
@@ -2496,7 +2528,13 @@
}
}
- public final void
+ /*
+ Note that this method is 'synchronized'--to force visibility
of all fields it affects.
+ Since it's called only once (per instance) for an MBean
Registration, it has no performance
+ impact on later use, but guarantees visibility of all non-
final instance variables, both
+ on this class and all subclasses, since they can only modify
things via postRegisterHook().
+ */
+ public final synchronized void
postRegister( Boolean registrationSucceeded )
{
super.postRegister( registrationSucceeded );
@@ -2540,7 +2578,7 @@
/**
The QueryMgr is a special-case; all the other types
rely on it.
*/
- public synchronized ObjectName
+ public ObjectName
getQueryMgrObjectName()
{
ObjectName objectName = null;
@@ -2609,17 +2647,16 @@
protected final QueryMgr
getQueryMgr()
{
- if ( mQueryMgr == null )
- {
+ // this relies on mQueryMgr being 'volatile'
+ if ( mQueryMgr != null )
+ return mQueryMgr;
+
final ObjectName objectName =
getQueryMgrObjectName();
if ( objectName != null )
{
- synchronized( this )
- {
+ // it doesn't matter if two thread do this; the same
proxy will be returned.
mQueryMgr = getProxyFactory().getProxy
( objectName, QueryMgr.class);
}
- }
- }
return( mQueryMgr );
}