Re: svn commit: r438785: move to ServiceContract/Operation

[Jim Marino]

On Sep 10, 2006, at 9:22 AM, Venkata Krishnan wrote:
HI Jim,

Thanks.  I have responded below.  Please let me know how to go  
forward with
this.

- Venkat

On 9/9/06, Jim Marino <[EMAIL PROTECTED]> wrote:

Hi Venkat,

Thanks for jumping in and doing this.

I was thinking the JDK proxy wire service would still remain as one
implementation of WireService; there would be a separate ASM-based
one that could be substituted since in some environments security
constraints will preclude the use of bytecode generation. Given that,
I was thinking this would be an optional service similar to the data
binding plugins or Jetty.

Some general comments on the patch:

1. A general overview and description of how things work would help
me understand things better.


I shall do this with JDKWireService as the starting point .
- The 'createProxy' method 'was' creating dynamic proxies (
java.lang.reflect.Proxy) is now modified to create instances of  
ProxyClasses
whose bytecode are generated during runtime.  Earlier, since the  
proxies
were generic, for every method invocation they needed to lookup a  
map and
obtain the corresponding invocation chain to invoke.  Now, with the  
bytecode
generated proxy, the chains are set into the proxy instance and the  
proxy
methods are implemented such that they invoke over the  
corresponding chains
directly.
- The ProxyFactory is the class that encapsulates all the function of
generting this byte code, then creating an instance of the class  
that is
defined by this byte code.
- The createProxy method is called on the JDKWireService either  
with an
inbound wire instance or an outbound wire instance.  For both cases  
the
following steps are followed: -
     - the interface of the wire is obtained using
wire.getServiceContract().getInterfaceClass()
and this will be the interface that the generated proxy will implement
     - each method of the wire's interface has its own invocation  
chain
instance, a map of InvocationChains is created where the keyset of  
this map
are the methods of this interface.  Since the proxy is to be  
generated such
that its methods diirecty invoke over the corresponding chain  
instances,
this map is used by the ProxyFactory to set the chains into the proxy
instance, with each chain being set into a specific field of the Proxy
instance.  The body of each method for this proxy is generated such  
that it
works with the corresponding 'chain' field.
     - The JDKInboundInvocationHandler & JDKOutboundInvocationHandler
contained all logic for invocation of methods on an Invocation  
chain.  To
retain this logic, this is passed to the proxy factory so that this  
'base
logic' is a part of the the base class for the generated proxy  
instance.
- Thus the proxy is generated instantiated and returned.


2. Why are spi and core classes modified, in particular removal of
the invocation chain specifications, e.g. InboundInvocationChain, and
replaces with the more generic InvocationChain? This ASM-based
service should just be a drop in.

- There is no change to any spi class.
- As for using InvocationChain instead of the specialized
InboundInvocationChain and OutboundInvocationChain, I did so  
because the
behaviour of JDKWireService, ProxyFactory did not require any  
knowledge of
the specializations.
I think we should stick with the goal of just providing a drop in  
replacement for the JDKWireService that is an optional extension.  
Let's hold off modifying any core or spi classes unless there is  
something in the design that does not make this possible. If we want  
to modify spi or core classes for improvements, then we should do  
that separately.
- The JDKInboundInvocationHanlder and the  
JDKOutboundInvocationHandler have
the same structure and behaviour (duplicated, unless I missed  
something). So
I have abstracted this common behaviour out into  
AbstractInvocationHandler
Yes I abstracted an invocation handler out about a week ago so you  
could extend that.
from which the classes AbstractInboundInvocationHandler and
AbstractOutboundInvocationHandler will inherit.  Thus the common  
duplicated
functionality is now available to both without duplication.  All of  
the
specialized behaviour (for inbound vs outbound) actually lies with the
AbstractInboundInvocationHandler and  
AbstractOutboundInvocationHandler.
These alone need to know if an invocation chain is Inbound or  
Outbound and
hence here I have left them that way.
- Though I have made these changes, I have not cropped the old code  
out of
apprehension that it might break some other code (though I'd be  
surprised if
it did :)).  So if you ok this, the I will crop them so that the  
whole stuff
looks cleaner.
I'm sorry but I'm not following you here. For a patch, smaller is  
better for me so I can follow the changes :-)



3. What is the purpose of WireProxyClasslaoder in ProxyFactory and
why does it use the TCCCL to generate classes?

Since the proxy's bytecode is generated, I needed to load this  
generated
bytecode.  For this I would required to call the 'define' method of  
the
class loader.  This method is protected and hence I can do it only  
if I had
my own class loader and override the 'define' method in it.  This  
is all
from whaever little I know.  Is there anyother way of doing this ?
We need to be careful about assuming things about the current  
classloader. For now, I would leave what you have but mark it with a  
TODO since we may need to hook this into the multip-parent classloader.


4. There are no test cases, in particular unit tests,  so I can't
verify if the patch works without writing them myself.


I think the only substantial add that might require separate test  
cases is
the ProxyFactory.  I will add that.  Other than this, it is the  
behaviour of
the existing classes that have been changed.  So my understand was  
that if I
could run all the existing testcases successfully that should be  
enough.
Won't it?
Not really. There are two types of tests we need to perform. Unit  
tests for things like ProxyFactory (these should test generation of  
various interfaces). We will also have to do a lot more "core  
integration tests" where we exercise invocation paths through the  
generated proxies. Here, these should use EasyMock to mock out  
invocation chains and test that the generated proxy dispatches to the  
correct chains. We won't need to test common functionality in the  
abstract super classes as that will be done elsewhere, but we need to  
be sure to test that methods dispatch to the correct invocation  
chain. We should cover tricky situations like overloaded methods  
since they are legal. Also, I would put negative tests in for  
scenarios where bad interface definitions occur.
And then, most testcases did work successfully but for the ones
that asserted if the created proxy was an instance of
java.lang.reflect.Proxy.  I could have change all them as well, but  
did not
want to go with too may changes all over the place until you  
confirmed that
I was on track.
We should change those tests as they test the JDK wire service. The  
tests for the ASM-based implementation should be separate.

5. The code is not formated correctly :-)


I will look into this.

Could we work on creating a separate module that contributes the ASM-
based wire service as an optional system service, does not modify
anything in core or SPI, has unit test coverage, and and passes -
Psourcheck (checkstyle and PMD)?


There is no change to SPI and the changes to the core should not  
break any
of the existing stuff or contaminate the existing abstractions.     
However,
if you can help me understand how to go about 'separate module' I  
will be
happy to do it.  Please let me know.
I just want to make sure we're on the same page that this is not a  
"complete replacement" for the JDK wire service (i.e. we will no  
longer use it), but that it is an optional implementation that may be  
substituted. To do this, I would look at one of the projects under  
services, such as Jetty, or databinding. I would place this service  
under services.

Thanks for doing this,
Jim


Thanks

- Venkat


Thanks,
Jim

On Sep 7, 2006, at 11:55 PM, Venkata Krishnan wrote:

> Hi Jim... did you get a chance to look at this. :)
>
> - Venkat
>
> On 9/4/06, Jim Marino <[EMAIL PROTECTED]> wrote:
>>
>> Great. I'll take a look.
>>
>> Jim
>>
>>
>> On Sep 4, 2006, at 2:14 AM, Venkata Krishnan wrote:
>>
>> > Hi Jim,
>> >
>> > I have posted a patch for this in
>> > http://issues.apache.org/jira/browse/TUSCANY-690.  Please take a
>> > look and
>> > let me know if I that is what you had in mind ;-).  Also let me
>> > know what
>> > more might need to be done.
>> >
>> > I have made quite a few changes to classes in the
>> > org.apache.tuscany.core.wire and
>> > org.apache.tuscany.core.wire.jdkpackages.   I have also added  
two
>> > classes...
>> >
>> > - AbstractInvocationHandler in the org.apache.tuscany.core.wire
>> > package to
>> > abstract out some common function related to chain holder and
>> > caching target
>> > invokers.
>> > - ProxyFactory in org.apache.tuscany.core.wire.jdk to create the
>> proxy
>> > instances.  This could probably be a good place to cache the
>> > bytecodes as
>> > well.
>> >
>> > Please let me know if this is what is in your mind and also  
let me
>> > know how
>> > to proceed further in this.
>> >
>> > Thanks
>> >
>> > - Venkat
>> >
>> > On 9/4/06, Venkata Krishnan <[EMAIL PROTECTED]> wrote:
>> >>
>> >> Hi Jim,
>> >>
>> >> Just to keep you updated.. I have stared on this and have  
got some
>> >> code up
>> >> for this.  I am currently working on getting it work. ;-)   
trying
>> >> to get
>> >> around with the ASM utilities and APIs.  Should be done with it
>> soon.
>> >>
>> >> Thanks
>> >>
>> >> - Venkat
>> >>
>> >>
>> >> On 9/2/06, Jim Marino <[EMAIL PROTECTED]> wrote:
>> >> >
>> >> > Sorry for the delay...
>> >> >
>> >> > On Aug 31, 2006, at 10:26 AM, Venkata Krishnan wrote:
>> >> >
>> >> > > Hi Jim,
>> >> > >
>> >> > > Please see my further queries below and let me know if my
>> >> thinking
>> >> > > is right.
>> >> > >
>> >> > > On 8/31/06, Jim Marino < [EMAIL PROTECTED]> wrote:
>> >> > >>
>> >> > >> I'd prefer ASM (CGLIB is built using it) since it is  
smaller,
>> >> easier
>> >> > >> to use (CGLIB documentation is awful) and will probably  
give
>> >> us more
>> >> > >> flexibility.
>> >> > >
>> >> > >
>> >> > > Venkat : Ok.. will use ASM.
>> >> > >
>> >> > >
>> >> > > What we need to do is provide an implementation of  
WireService
>> >> that
>> >> > >> uses an approach which dispatches method invocations  
directly
>> >> to the
>> >> > >> correct invocation chain. The JDK wire service currently
>> creates
>> >> > >> invocation handlers that resolve the chain to invoke  
based on
>> >> the
>> >> > >> method passed in from the proxy. We should be able to
>> >> generate proxy
>> >> > >> code that does a "static" invoke to the correct chain  
without
>> >> the
>> >> > >> overhead of a map lookup (see the JDK-based
>> implementations of
>> >> > >> invocation handler).
>> >> > >
>> >> > >
>> >> > > I follow this.  So here is what I understand as, to be  
done:
>> >> > > - for the logic in  'createProxy' method of the  
JDKWireService
>> >> > > generate a
>> >> > > class using ASM that implements the wire's interface -  
let us
>> >> call
>> >> > > this
>> >> > > 'generatedProxy'
>> >> > I separated out common logic into
>> AbstractInboundInvocationHandler
>> >> > and AbstractOutboundInvocationHandler so you should be able
>> >> subclass
>> >> > those. We will eventually have to do a callback invocation
>> >> handler as
>> >> > well but this is in flux so I would concentrate on the two
>> for now.
>> >> >
>> >> > > - depending on the number of methods, create as many  
fields in
>> >> this
>> >> > > generatedProxy to hold instances of  the InvocationChain
>> >> > > - implement the body of each method in this
>> 'generatedProxy' to
>> >> > > execute all
>> >> > > of the logic in JDKOutboundInvocationHandler (or other  
inbound
>> >> > > hanlder as
>> >> > > the case may be) using the appropriate field that holds its
>> >> > > corresponding
>> >> > > 'InvocationChain' instance.
>> >> > >
>> >> > If you subclass the new abstract classes, you will just  
have to
>> >> > implement the indexing mentioned above.
>> >> >
>> >> > > Having generated the bytecode for this 'generatedProxy'
>> create an
>> >> > > instance
>> >> > > of it and return that as return value for the 'createProxy'
>> >> method.
>> >> > >
>> >> > > Is this right?
>> >> > >
>> >> > Yep. We may also want to profile performance of the
>> generation and
>> >> > see if we want to do caching of generated bytecode. I'm  
going to
>> >> ask
>> >> > the Aspectwerkz guys about this since they used ASM in a  
similar
>> >> way
>> >> > and did extensive performance analysis.
>> >> >
>> >> > Jim
>> >> >
>> >> > >
>> >> > > In addition, we should be able to create
>> >> > >> optimized variants of WireInvocationHandler. We  
particularly
>> >> need to
>> >> > >> optimize JDKCallbackInvocationHandler and the
>> >> > >> JDKOutboundInvocationHandler constructor.
>> >> > >>
>> >> > >> As a note, I still need to switch over
>> >> WireInvocationHandler.invoke
>> >> > >> (Method method, Object[] args) to take an Operation
>> instead of a
>> >> > >> method.
>> >> > >>
>> >> > >> Geronimo I believe uses something similar so perhaps  
Jeremy
>> >> could
>> >> > >> describe it further? I've also seen this done in  
Aspectwerkz
>> >> with
>> >> > >> extremely fast performance.
>> >> > >>
>> >> > >> Jim
>> >> > >
>> >> > >
>> >> > > As an when I get info on this I shall make the relevant
>> changes.
>> >> > > Is that ok
>> >> > > ?
>> >> > >
>> >> > > Thanks
>> >> > >
>> >> > > - Venkat
>> >> > >
>> >> > >
>> >> > > On Aug 31, 2006, at 12:04 AM, Venkata Krishnan wrote:
>> >> > >>
>> >> > >> > Hi Jim,
>> >> > >> >  I worked a bit with cglib to generate manipulated
>> >> intefaces as
>> >> > >> > apart of
>> >> > >> > the RMI-Binding and JavaScript e4x stuff.  If this is  
good
>> >> for a
>> >> > >> > start, then
>> >> > >> > please let me know what is to be done in JDKWireService.
>> >> Thanks
>> >> > >> >
>> >> > >> > - Venkat
>> >> > >> >
>> >> > >> > On 8/31/06, Jim Marino < [EMAIL PROTECTED]> wrote:
>> >> > >> >>
>> >> > >> >> I have committed the changes to use ServiceContract/
>> >> Operation in
>> >> > >> >> place of Java Interface/Method. This entailed
>> >> modifications in
>> >> > >> SPI,
>> >> > >> >> particularly the signatures for classes in the wire
>> >> package (e.g.
>> >> > >> >> RuntimeWire, InvocationChain), WireService, and  
Component
>> >> > >> >> (createTargetInvoker).
>> >> > >> >>
>> >> > >> >> ServiceContracts are created by an introspection  
registry
>> >> for a
>> >> > >> given
>> >> > >> >> IDL. These registries may use the same pattern as
>> >> registries for
>> >> > >> >> loaders and builders. For example, Java IDL has a
>> >> > >> >> JavaInterfaceProcessorRegistryImpl that delegates to
>> >> > >> >> JavaInterfaceProcessor implementations to populate a
>> >> > >> ServiceContract
>> >> > >> >> reflected from a Java interface. JavaInterfaceProcessor
>> >> > >> >> implementations may be system services which register
>> with a
>> >> > >> >> JavaInterfaceProcessorRegistry, thereby adding  
support for
>> >> > >> metadata
>> >> > >> >> extensions (such as information specific to a  
databinding
>> >> type).
>> >> > >> >>
>> >> > >> >> ServiceContract metadata is used to decorate wires.
>> >> > >> RuntimeWire has
>> >> > >> >> a new method getServiceContract() which will return the
>> >> service
>> >> > >> >> contract associated with an inbound or outbound wire.
>> >> Similarly,
>> >> > >> >> Invocation chains are decorated with an Operation. This
>> >> > >> metadata will
>> >> > >> >> be used by a new system service responsible for
>> "optimizing/
>> >> > >> >> mediating" wires as they are connected by the wiring
>> >> > >> infrastructure.
>> >> > >> >> This system service will be another registry with a  
set of
>> >> > >> builders.
>> >> > >> >> It will be different than the policy builder registry
>> >> since policy
>> >> > >> >> builders only operate on one side of a wire (source or
>> >> target).
>> >> > >> >> Raymond's databinding framework will use these
>> facilities to
>> >> > >> >> introspect the outbound and inbounds parts of a wire
>> and add
>> >> > >> >> appropriate interceptors.
>> >> > >> >>
>> >> > >> >> Currently, Operation and ServiceContract do not have
>> >> facilities
>> >> > >> for
>> >> > >> >> runtime extensibility metadata; I will be adding these
>> along
>> >> > >> with the
>> >> > >> >> new type of "optimizing/mediating" registry (probably)
>> >> tomorrow.
>> >> > >> >>
>> >> > >> >> One outstanding work item is to provide a performant
>> >> > >> replacement for
>> >> > >> >> JDK proxies (JDKWireService). I suspect we will see
>> >> significant
>> >> > >> >> performance penalties for local invocations,  
particularly
>> >> > >> callbacks,
>> >> > >> >> as several map lookups are done. If anyone is
>> interested in
>> >> > >> creating
>> >> > >> >> an implementation of WireService based on a bytecode
>> >> generation
>> >> > >> >> library such as ASM, please let me know and I will  
try and
>> >> provide
>> >> > >> >> pointers.
>> >> > >> >>
>> >> > >> >> Jim
>> >> > >> >>
>> >> > >> >>
>> >> > >> >>
>> >> > >>
>> >>
>>  
---------------------------------------------------------------------
>> >> > >> >> To unsubscribe, e-mail: tuscany-dev-
>> [EMAIL PROTECTED]
>> >> > >> >> For additional commands, e-mail: tuscany-dev-
>> >> [EMAIL PROTECTED]
>> >> > >> >>
>> >> > >> >>
>> >> > >>
>> >> > >>
>> >> > >>
>> >>
>>  
---------------------------------------------------------------------
>> >> > >> To unsubscribe, e-mail: tuscany-dev- 
[EMAIL PROTECTED]
>> >> > >> For additional commands, e-mail: tuscany-dev-
>> [EMAIL PROTECTED]
>> >> > >>
>> >> > >>
>> >> >
>> >> >
>> >> >
>> >>
>>  
---------------------------------------------------------------------
>> >> > To unsubscribe, e-mail: [EMAIL PROTECTED]
>> >> > For additional commands, e-mail: tuscany-dev- 
[EMAIL PROTECTED]
>> >> >
>> >> >
>> >>
>>
>>
>>  
---------------------------------------------------------------------
>> To unsubscribe, e-mail: [EMAIL PROTECTED]
>> For additional commands, e-mail: [EMAIL PROTECTED]
>>
>>


---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]




---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]