aries-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alasdair Nottingham <...@apache.org>
Subject Re: Customising the behaviour of the weaving proxy implementation
Date Tue, 07 Feb 2012 14:19:14 GMT
Hi,

In my updated proposal the method would better be called shouldWeaveClass
given I updated it to take a WovenClass rather than a Bundle.

The reason I am concerned about the overhead is because I know in my
scenario the computation will always return false. That means I will take a
hit on every class loader and in my environment that is a lot of classes.
Performance is really important and I want to remove as much extra
processing as is required.

I think writing a ProxyWeavingController is an advanced capability and
people need to do it correctly, if someone gets it wrong that isn't our
issue (after all people can cause huge issues if they badly write the
plugins to application install, or blueprint). To help people get this
right we could easily provide an abstract base class which performs the
validation for people who need it, but for those who don't they can get the
best performance possible in their scenario.

The scenario I have in mind is one where I want weaving for bundles in .eba
applications, but not for bundles installed by a different provisioner.
Given the way our .eba isolation model works that means I just need a
weaving hook that returns true for bundles installed via the application
module, and false otherwise.

Alasdair

On 7 February 2012 10:26, Timothy Ward <timothyjward@apache.org> wrote:

>
> Hi,
>
> I'm not certain that I see the check I describe as particularly expensive,
> the following code would be enough:
>
>
>    ClassReader cReader = new ClassReader(wovenClass.getBytes());
>    boolean mustBeWoven;
>    try {
>        Class<?> superClass =
> Class.forName(cReader.getSuperName().replace('/', '.'), false,
>                wovenClass.getBundleWiring().getClassLoader());
>        mustBeWoven = WovenProxy.class.isAssignableFrom(superClass);
>    } catch (ClassNotFoundException e) {
>        String failureMessage =
> NLS.MESSAGES.getMessage("fatal.weaving.failure", name);
>        //This is a failure that should stop the class loading!
>        LOGGER.error(failureMessage, e);
>        throw new WeavingException(failureMessage, e);
>    }
>
>    boolean weave = mustBeWoven || ....
>
>
> The overhead of adding this code is limited to: creating a ClassReader
> from a live byte array (very fast, nothing copied just a few constant pool
> entries read) and reading a few entries from a byte array to build the
> super class name String. The replace operation is also very fast, as it
> only needs to do some char comparison on a String that is unlikely to be
> more than 100 chars long.
>
> The only moderately expensive operation in the entire process is when we
> load the superclass, however this will need to happen anyway when the
> subclass is loaded, so it isn't wasted effort. As this code only needs to
> exist once in the proxy weaving hook it also doesn't affect scaling. I see
> the safety benefit that this code adds as a significant advantage at a very
> low performance cost.
>
> Yes, the existing filtering approach exhibits the same problem that I've
> described for the proposed one. I raised this same issue at the time, but
> didn't have an immediate fix in mind. Now that I do have one I'd really
> like to fix that filtering too.
>
>
> As for the change you suggest to the ProxyWeavingController, it certainly
> gives the implementation of the filter more information to play with,
> although I think the method name should be something like
> "shouldWeaveBundle" to make sure that people who don't read the JavaDoc
> aren't so surprised when they only get called for the first class loaded by
> a given bundle and the result is cached (rather than once for every
> WovenClass). Other than that, more information is usually a good thing :)
>
> Regards,
>
> Tim Ward
> -------------------
> Apache Aries PMC member & Enterprise OSGi advocate
> Enterprise OSGi in Action (http://www.manning.com/cummins)
> -------------------
>
>
> > Date: Mon, 6 Feb 2012 22:08:56 +0000
> > Subject: Re: Customising the behaviour of the weaving proxy
> implementation
> > From: not@apache.org
> > To: dev@aries.apache.org
> >
> > Hi,
> >
> > I'm a little reluctant to do what you propose. In my scenario I can't get
> > into the situation you describe and the overhead of validating the class
> > hierarchy worries me for the scenario I have. I would also point out that
> > the current baked in approach that disable certain classes or bundles
> > suffers from the same issues.
> >
> > Instead I'm wondering about the following "major" modification to the
> > interface:
> >
> > package org.apache.aries.proxy.weaving;
> >
> >
> > import org.osgi.framework.Bundle;
> >
> > import org.osgi.framework.hooks.weaving.WovenClass;
> >
> >
> > /**
> >
> >  * Services of this interface are used by the ProxyManager's weaving
> > implementation to
> >
> >  * decide if a specific bundle should be subject to weaving.
> >
> >  */
> >
> > public interface ProxyWeavingController
> >
> > {
> >
> >   /**
> >
> >    * Returns true if the bundle should be subject to proxy weaving. If it
> > returns
> >
> >    * false then the bundle will not be weaved. The result of this method
> is
> > immutable
> >
> >    * for a given bundle. That means repeated calls given the same bundle
> > MUST
> >
> >    * return the same response.
> >
> >    *
> >
> >    * @param clazz the class that is to be weaved
> >
> >    * @return true if it should be woven, false otherwise.
> >
> >    */
> >
> >   public boolean shouldWeave(WovenClass clazz);
> >
> > }
> >
> > This would allow me to do what I need efficiently, it would also allow
> use
> > cases I haven't considered. Then if someone returns false in the case you
> > describe they can write the code and take the expense of ensuring the
> right
> > consistency up the hierarchy.
> >
> > Thoughts?
> > Alasdair
> >
> > On 6 February 2012 17:25, Timothy Ward <timothyjward@apache.org> wrote:
> >
> > >
> > > Hi,
> > >
> > > I like the principle behind this change, and think it probably is a
> good
> > > way to go. It's lightweight, and results can be cached. There is one
> > > negative point that affects this (and in fact almost all) filtering,
> wihch
> > > is that the the weaving code is built around the fact that we weave
> > > consistently throughout the class hierarchy up to a certain ancestor
> > > (preferably java.lang.Object). I find this quite difficult to explain
> well
> > > in words, so forgive the pretty trivial example.
> > >
> > >
> > > We have three classes: Foo extends Bar, Bar extends Jimmy, and Jimmy
> > > extends Object.
> > >
> > > When we are weaving Foo, the first thing we do is to load Bar. If Bar
> can
> > > be woven then we will start by loading Jimmy. If Jimmy can be woven
> then we
> > > start by loading Object (which cannot be woven).
> > >
> > > If all three classes can be woven then we add hooks into every class,
> > > which is fine. If we can't weave Jimmy (which sometimes happens), then
> when
> > > we weave Bar we can detect that Jimmy wasn't woven (because it doesn't
> > > implement WovenProxy), and add overrides in Bar for any methods that it
> > > inherits from Jimmy to allow us to weave in code. This means that Jimmy
> > > can't be proxied (by the weaving code) but that Foo and Bar can.
> > >
> > > The problem is that this detection logic only spots the first break in
> the
> > > weaving chain, and not subsequent breaks.
> > >
> > > If we were able to weave Foo and Jimmy, but not Bar, then the weaving
> code
> > > would not know to override the methods that Foo inherits from Bar (Bar
> > > implements WovenProxy because Jimmy does). Even worse, the weaving code
> > > adds some synthetic methods to every class in the weaving hierarchy.
> These
> > > will be missing from Bar, and use the implementations from Jimmy
> instead,
> > > which will give the wrong answer. Also, because Bar will still
> implements
> > > the WovenProxy interface the proxy bundle will think that Bar is
> proxyable,
> > > even though it actually isn't.
> > >
> > >
> > > I don't necessarily see this issue as a dealbreaker for the proposed
> > > solution, but I do want people to be aware of the risks associated with
> > > only weaving a subset of classes in a hierarchy.
> > >
> > > We could potentially prevent this situation by ignoring the bundle-wide
> > > filter if the superclass of the class to be woven already implements
> > > WovenProxy. This would mean that some classes from "unweavable" bundles
> > > would be woven, but only if they were already part of a woven hierarchy
> > > from some other bundle. It seems highly unlikely that this would affect
> > > Alasdair's scenario (which I imagine is the common use case), but it
> also
> > > prevents us from ever violating the assumptions made by the weaving
> code
> > > about the continuity of the woven methods.
> > >
> > > All in all, a tentative +1. Alasdair, what do you think of a minor
> tweak
> > > to override the filter in the case I describe above? I think it will
> be a
> > > line or two change to your existing code, plus a very simple
> ClassVisitor.
> > > We could even move some of the code out of the visit method of
> > > AbstractWovenProxyAdapter and pass the super class as a java.lang.Class
> > > constructor parameter to avoid duplicating effort. With this change I
> would
> > > have no concerns about the solution at all.
> > >
> > > Regards
> > >
> > > Tim Ward
> > > -------------------
> > > Apache Aries PMC member & Enterprise OSGi advocate
> > > Enterprise OSGi in Action (http://www.manning.com/cummins)
> > > -------------------
> > >
> > >
> > > > Date: Sat, 4 Feb 2012 23:43:38 +0000
> > > > Subject: Customising the behaviour of the weaving proxy
> implementation
> > > > From: not@apache.org
> > > > To: dev@aries.apache.org
> > > >
> > > > Hi,
> > > >
> > > > I have been consuming the proxy bundle with the weaving support for a
> > > while
> > > > now, however it results in weaving classes that will never be proxies
> > > ever.
> > > > In fact I want to "weave" the classes that live in applications, but
> not
> > > my
> > > > runtime. I would like to be able to disable weaving for bundles in my
> > > > runtime.
> > > >
> > > > I have prototyped a change locally which involves providing a new
> > > service:
> > > >
> > > > package org.apache.aries.proxy.weaving;
> > > >
> > > >
> > > > import org.osgi.framework.Bundle;
> > > >
> > > >
> > > > /**
> > > >
> > > >  * Services of this interface are used by the ProxyManager's weaving
> > > > implementation to
> > > >
> > > >  * decide if a specific bundle should be subject to weaving.
> > > >
> > > >  */
> > > >
> > > > public interface ProxyWeavingController
> > > >
> > > > {
> > > >
> > > >   /**
> > > >
> > > >    * Returns true if the bundle should be subject to proxy weaving.
> If it
> > > > returns
> > > >
> > > >    * false then the bundle will not be weaved. The result of this
> method
> > > is
> > > > immutable
> > > >
> > > >    * for a given bundle. That means repeated calls given the same
> bundle
> > > > MUST
> > > >
> > > >    * return the same response.
> > > >
> > > >    *
> > > >
> > > >    * @param b the bundle that is being weaved
> > > >
> > > >    * @return true if it should be woven, false otherwise.
> > > >
> > > >    */
> > > >
> > > >   public boolean shouldWeave(Bundle b);
> > > >
> > > > }
> > > >
> > > > I've updated the proxy weaving support to call services and only if
> they
> > > > all agree that the bundle should be woven will it be woven. I'd like
> to
> > > > commit this to the codebase, but I wanted to get peoples thoughts
> before
> > > I
> > > > did.
> > > >
> > > > Thanks
> > > > Alasdair
> > > >
> > > > --
> > > > Alasdair Nottingham
> > > > not@apache.org
> > >
> > >
> >
> >
> >
> > --
> > Alasdair Nottingham
> > not@apache.org
>
>



-- 
Alasdair Nottingham
not@apache.org

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message