DESCRIBE phase - Dependency builders for MC Beans

[jaikiran]

While looking into the deployment timings issues, i noticed that any MC bean that gets deployed spends (relatively) considerable amount of time during the DESCRIBE phase. A bit of debugging reveals that the DescribeAction relies on a dependency builder which is configured at the kernel level:

org.jboss.kernel.plugins.dependency.DescribeAction:

 DependencyBuilder dependencyBuilder = config.getDependencyBuilder();
 KernelMetaDataRepository repository = controller.getKernel().getMetaDataRepository();
 MetaData md = repository.getMetaData(context);
 // add custom dependencies (e.g. AOP layer).
 List<DependencyBuilderListItem> dependencies = dependencyBuilder.getDependencies(info, md);

By *default* the dependency builder is set to AOPDependencyBuilder

KernelConstants:

static final String DEPENDENCY_BUILDER_DEFAULT="org.jboss.aop.microcontainer.integration.AOPDependencyBuilder:org.jboss.kernel.spi.dependency.helpers.AbstractDependencyBuilder";

This AOPDependencyBuilder uses an AspectManager to try and apply any AOP bindings to *all* MC beans that get deployed:

 * Finds all managed aspects that apply
 * to the bean and includes their dependencies as dependencies of the bean
 *
public class AOPDependencyBuilder extends AbstractDependencyBuilder
{

Effectively this dependency builder does AOP stuff by applying bindings defined in litterally every *aop-beans.xml in the server. Aspects/bindings defined in AS/common/lib/jboss-messaging.jar are also picked up. Overall, every MC beans goes through this AOPDependencyBuilder and (expensive) AOP stuff is carried out on these MC beans. But from what i see, most of the MC beans being deployed don't rely about this AOPDependencyBuilder to generate the dependencies. Atleast EJB3 container deployment MC beans don't rely on this. I hacked into this component to ensure that this is not a default dependency builder and was able to boot most of the AS (expect i think the webserver deployment, iirc) without issues, which means that *not* having this as the default dependency builder probably is a better option.

Given the way, this dependency builder is currently picked up (it's picked up from the KernelConfig which corresponds to the Kernel), i found no way of configuring a different dependency builder (which just returns null on the lines of org.jboss.kernel.spi.dependency.helpers.AbstractDependencyBuilder) for the EJB3 MC bean deployments. Changing the default is going to affect other (few) deployments.

1) Can the AOPDependencyBuilder be removed as the default
2) Currently this is a kernel level property. Can this be made per bean/context level property (by default set to org.jboss.kernel.spi.dependency.helpers.AbstractDependencyBuilder) so that each MC bean context can override this if required? Whichever MC beans need this AOPDependencyBuilder currently in the AS can then be switched appropriately.
3) This is not directly related to this issue - But any reason why the configs defined in jar files under AS/common/lib (aop-messaging-client.xml and aop-messaging-server.xml) are being parsed? I did not debug much into this part.

Just to see how much improvement this change, might bring in to (EJB3) deployments, i hacked the AOPDependencyBuilder to skip the AOP stuff for org.jboss.ejb3.* MC deployments. My sample application which was taking 20 seconds to deploy. now takes 15 seconds - still on a higher side but there's a 25% improvement by this one single hack.

[alesj]

"jaikiran" wrote:

1) Can the AOPDependencyBuilder be removed as the default

No, as this would break all the @JMX definitions.
Plus probably a ton of other stuff. ;-)

"jaikiran" wrote:

2) Currently this is a kernel level property. Can this be made per bean/context level property (by default set to org.jboss.kernel.spi.dependency.helpers.AbstractDependencyBuilder) so that each MC bean context can override this if required? Whichever MC beans need this AOPDependencyBuilder currently in the AS can then be switched appropriately.

I'll see what we can do.
I'm thinking in the lines of putting DependencyBuilder into per bean instance scope in MetaData.
If there is one present, use it, otherwise fall to default - from KernelConfig.

[alesj]

"alesj" wrote:

I'm thinking in the lines of putting DependencyBuilder into per bean instance scope in MetaData.
If there is one present, use it, otherwise fall to default - from KernelConfig.

I've added this:

 protected DependencyBuilder getDependencyBuilder(MetaData md, Kernel kernel) throws Throwable
 {
 DependencyBuilder dependencyBuilder = md.getMetaData(DependencyBuilder.class);
 if (dependencyBuilder == null)
 {
 KernelConfig config = kernel.getConfig();
 dependencyBuilder = config.getDependencyBuilder();
 }
 return dependencyBuilder;
 }

 @SuppressWarnings("unchecked")
 protected void installActionInternal(KernelControllerContext context) throws Throwable
 {
 BeanInfo info = context.getBeanInfo();
 if (info != null)
 {
 KernelController controller = (KernelController)context.getController();
 Kernel kernel = controller.getKernel();
 KernelMetaDataRepository repository = kernel.getMetaDataRepository();
 MetaData md = repository.getMetaData(context);
 DependencyBuilder dependencyBuilder = getDependencyBuilder(md, kernel);
 // add custom dependencies (e.g. AOP layer).
 List<DependencyBuilderListItem> dependencies = dependencyBuilder.getDependencies(info, md);

can you try it out?

You will need to take an existing KernelControllerContext
and add DependencyBuilder metadata for its ScopeInfo's scope.
(see BasicKernelMetaDataRepository for actual usage)

This will (mostly) even add more penalty to the performance,
as we have to check MetaData for DependencyBuilder instance.
Although it shouldn't be much, as it's only a few Map entry checks.

[jaikiran]

"alesj" wrote:

can you try it out?

Sure, will try it out today.

"alesj" wrote:

This will (mostly) even add more penalty to the performance,
as we have to check MetaData for DependencyBuilder instance.
Although it shouldn't be much, as it's only a few Map entry checks.

I haven't seen the internal implementation, but i believe, this should be far better than the AOP stuff that was done for the MC beans. I'll try out a simple test to see how much improvement this brings in.

[jaikiran]

In the meantime,

"alesj" wrote:

"jaikiran" wrote:

1) Can the AOPDependencyBuilder be removed as the default

No, as this would break all the @JMX definitions.
Plus probably a ton of other stuff. ;-)

I agree, there's going to be some stuff which going to be break (till we fix it :) ). But in the long term, shouldn't we be moving away from this default? Here's why i think the AOPDependencyBuilder default might not be a good idea:

1) For non-programmatic MC bean deployments (i.e. just drop the -beans.xml file), there's no way to override this default dependency builder

2) Even for programmatic MC bean deployments, not everyone will be aware of this issue (or the internal implementation of MC), so most of the times, they are going to end up without any overridden (better performing) dependency builder in the metadata

3) If this dependency builder is for beans that use @JMX or something similar, then i guess there might be some place where this AOPDependencyBuilder can be attached/injected internally by MC to ensure that only such deployments are handled by this builder?

[kabirkhan]

"jaikiran" wrote:

1) For non-programmatic MC bean deployments (i.e. just drop the -beans.xml file), there's no way to override this default dependency builder

An annotation could be used on the deployment or per bean, e.g. @NoAop

[alesj]

"jaikiran" wrote:
But in the long term, shouldn't we be moving away from this default?

No, that's why it's called *transparent* AOP integration. ;-)

You can only argue that the AOP mechanism is not performant enough,
but that's as far as I agree with you.
Although this comes with a cost, it does bring lots of nice features.
They are mostly true in a sense of AOP; aspects, concerns separation, ...
@JMX, @JNDI, @Password, @(add-your-own-metadata-here) are good examples of this.
And if I was a user, that's what I would want too, out-of-the-box.

But yeah, there should be a way to explicitly avoid this,
and this is/can be achieved with this new DB per MD mechanism.
And if there is a real demand for this "outside" programmatic usage,
it's trivial to add new declarative way of using this.

[alesj]

"alesj" wrote:

And if there is a real demand for this "outside" programmatic usage,
it's trivial to add new declarative way of using this.

This being one of them. :-)

An annotation could be used on the deployment or per bean, e.g. @NoAop

[alesj]

"alesj" wrote:

"alesj" wrote:

And if there is a real demand for this "outside" programmatic usage,
it's trivial to add new declarative way of using this.

This being one of them. :-)

An annotation could be used on the deployment or per bean, e.g. @NoAop

Here, more stuff to test. :-)

 /** Basic dependency builder, no AOP */
 private DependencyBuilder basicDependencyBuilder = createBasicDependencyBuilder();

 /**
 * Create basic dependency builder.
 *
 * @return the basic dependency builder
 */
 protected DependencyBuilder createBasicDependencyBuilder()
 {
 return new AbstractDependencyBuilder();
 }

 /**
 * Get dependency builder.
 *
 * @param md the metadata
 * @param kernel the kernel
 * @return dependency builder
 * @throws Throwable for any error
 */
 protected DependencyBuilder getDependencyBuilder(MetaData md, Kernel kernel) throws Throwable
 {
 DependencyBuilder dependencyBuilder = md.getMetaData(DependencyBuilder.class);
 if (dependencyBuilder == null)
 {
 IgnoreAOP ignoreAOP = md.getAnnotation(IgnoreAOP.class);
 if (ignoreAOP != null)
 {
 return basicDependencyBuilder;
 }
 else
 {
 KernelConfig config = kernel.getConfig();
 dependencyBuilder = config.getDependencyBuilder();
 }
 }
 return dependencyBuilder;
 }

Or we could have @DependencyBuilderMarker, where you would define a DB class?

[jaikiran]

"alesj" wrote:

 DependencyBuilder dependencyBuilder = md.getMetaData(DependencyBuilder.class);
 if (dependencyBuilder == null)
 {
 IgnoreAOP ignoreAOP = md.getAnnotation(IgnoreAOP.class);
 if (ignoreAOP != null)
 {
 return basicDependencyBuilder;

Or we could have @DependencyBuilderMarker, where you would define a DB class?

I guess, instead of checking two things in the metadata (a DependencyBuilder and a IgnoreAOP annotation), its better to just look for a single annotation which could be something along these lines (just some pseudo code):

@DependencyBuilder(value=DescribeAction.BASIC)
public class MyMCBean
{
...

public @interface DependencyBuilder
{
 String value();

DescribeAction
{

public static final String BASIC = "org...blah..AbstractDependencyBuilder";

public static final String AOP = "org.blah.AOPDependencyBuilder";

[jaikiran]

"alesj" wrote:

"jaikiran" wrote:
But in the long term, shouldn't we be moving away from this default?

No, that's why it's called *transparent* AOP integration. ;-)

You can only argue that the AOP mechanism is not performant enough,
but that's as far as I agree with you.
Although this comes with a cost, it does bring lots of nice features.
They are mostly true in a sense of AOP; aspects, concerns separation, ...
@JMX, @JNDI, @Password, @(add-your-own-metadata-here) are good examples of this.
And if I was a user, that's what I would want too, out-of-the-box.

I am not against using AOP, where its needed :) In the current scenario, there was no way to tell MC that i did not want AOP

"alesj" wrote:

But yeah, there should be a way to explicitly avoid this,
and this is/can be achieved with this new DB per MD mechanism.
And if there is a real demand for this "outside" programmatic usage,
it's trivial to add new declarative way of using this.

An annotation could be used on the deployment or per bean, e.g. @NoAop

Yes, this now allows us a way to skip/override the defaults.

[jaikiran]

value=DescribeAction.BASIC

These constants probably need to come from some place else assuming DescribeAction is an internal impl detail of the MC. But i believe, you get what i was trying to convey through that example :)

[alesj]

"jaikiran" wrote:

I guess, instead of checking two things in the metadata (a DependencyBuilder and a IgnoreAOP annotation), its better to just look for a single annotation

Yes, I was just thinking about this duplicate lookup.
This is what we have now:

 protected DependencyBuilder getDependencyBuilder(MetaData md, Kernel kernel) throws Throwable
 {
 DependencyBuilder dependencyBuilder = null;
 DependencyBuilderFactory factory = md.getAnnotation(DependencyBuilderFactory.class);
 if (factory != null)
 {
 if (factory.checkMetaDataForBuilderInstance())
 {
 // still allow for more configurable DependencyBuilder
 dependencyBuilder = md.getMetaData(DependencyBuilder.class);
 }
 else
 {
 Class<? extends DependencyBuilder> value = factory.value();
 if (basicDependencyBuilder.getClass().equals(value))
 {
 dependencyBuilder = basicDependencyBuilder;
 }
 else
 {
 dependencyBuilder = value.newInstance();
 }
 }
 }

 if (dependencyBuilder == null)
 {
 KernelConfig config = kernel.getConfig();
 dependencyBuilder = config.getDependencyBuilder();
 }

 return dependencyBuilder;
 }

[jaikiran]

if (factory.checkMetaDataForBuilderInstance())
 {
 // still allow for more configurable DependencyBuilder
 dependencyBuilder = md.getMetaData(DependencyBuilder.class);
 }

Do we still need to check the metadata? Just thinking whether its worth it, since it still leads to two checks.

[alesj]

"jaikiran" wrote:

if (factory.checkMetaDataForBuilderInstance())
 {
 // still allow for more configurable DependencyBuilder
 dependencyBuilder = md.getMetaData(DependencyBuilder.class);
 }

Do we still need to check the metadata? Just thinking whether its worth it, since it still leads to two checks.

This allows us for more configurable DB, not only no-arg constructor,
but perhaps a fully configurable, singleton, ... DB.

As you can see, this needs an explicit flag in order to check the MD,
hence it should be rarely true --> checked.