Thanks for getting these performance numbers. They confirm that
an optimization for this case is needed. The question is whether
the current patch is the right fix, and also whether now is the
right time to apply it.
Consider the following case, which could also be a common wiring
pattern. A and B are componments with STATELESS implementation
scopes. A has a reference to a service in B, and B has a
reference to a service in A (both normal forward references,
not callbacks).
When an instance of A is created, with the optimization, the
stateless scope container will create a new instance of B.
Creating the B instance will create another instance of A, which
will create another B instance and so on.... until we run out
of stack, heap or both.
This is just one example of a problem with the current code.
Sebastien has found a few issues with it as well, as well as
areas where we don't have good enough test coverage to pick up
all the problems that this change could create. There's probably
no test for my scenario above either. How many more issues are
there as well as those that have come up so far?
I think we would be taking too much of a risk if we do this now so
close to 1.0. It's a small change in terms of lines of code but
it raises big issues in terms of design. It needs more time for
careful consideration and analysis of all the scenarios.
Simon
Jean-Sebastien Delfino wrote:
Comments at the bottom
Raymond Feng wrote:
Yes, we check the presence of any other inteceptors than the java
implementation invoker in the chain for each operation. We optimize
only if java implementation invoker is the only one.
Thanks,
Raymond
----- Original Message ----- From: "scabooz" <[EMAIL PROTECTED]>
To: <[email protected]>
Sent: Thursday, September 13, 2007 5:18 PM
Subject: Re: Optimize the reference injection for java components
Yes, I agree. The only risk of doing this for stateless
components is that an improperly coded implementation
could malfunction due to left over state from the previous use.
Getting a clean instance every time will cover up the error.
I'm not trying to dissuade you from doing it, just pointing out
potholes.
The other question I forgot to ask was about policy. Does
the invocation handler check you proposed ensure that policy
interceptors will get control between components?
Dave
----- Original Message ----- From: "Raymond Feng" <[EMAIL PROTECTED]>
To: <[email protected]>
Sent: Thursday, September 13, 2007 5:12 PM
Subject: Re: Optimize the reference injection for java components
Even for the stateless case, I would argue it's legal to do the
optimization.
The SCA java spec says:
"283 1.2.4.1. Stateless scope
284 For stateless components, there is no implied correlation
between service requests."
I think it should also be valid to get the same instances for
multiple requests. For JEE stateless sessions, the container usally
maintains an instance pool and pick the instance from pool as
needed. So there is only one request at a time, then there is a good
chance that the same instance is picked.
My understanding of "stateless" is that there is no garauntee that
multiple requests will be routed to the same instance.
Thanks,
Raymond
----- Original Message ----- From: "scabooz" <[EMAIL PROTECTED]>
To: <[email protected]>
Sent: Thursday, September 13, 2007 1:57 PM
Subject: Re: Optimize the reference injection for java components
Comments inline
----- Original Message ----- From: "Mike Edwards"
<[EMAIL PROTECTED]>
To: <[email protected]>
Sent: Thursday, September 13, 2007 11:44 AM
Subject: Re: Optimize the reference injection for java components
Simon,
Some comments inline....
Simon Nash wrote:
If I understand this correctly, it would affect the lifecycle of
the target component instance. So when A has a reference to B,
the creation of A currently involves creating and injecting a B
proxy
but not a B instance. With this change, I think the creation of A
would involve creating a B instance and injecting a reference to
this
B instance into A. And if B references C, this would in turn
involve
creating a C instance and injecting its reference into B (and so
on).
I think there could be many consequences of this change, some of
which
may not be desirable. Creation on first invocation is easy to
understand
and consistent, and I'd be concerned about changing this.
But creation can occur at other times. Scope controls this -
COMPOSITE scope implies creation at deployment time, for example.
So you need to be able to deal with these cases anyway. Creation
on first invocation sounds like only one possible behaviour that
must be handled.
Since Raymond has already constrained the scenario to local,
injection of the
instance should be ok for REQUEST, CONVERSATION and COMPOSITE
because they should all be "singletons" during the life of the
request that is
currently in flight. For STATELESS, you're supposed to get a new
instance
on every invocation, so I don't think you'd want the optimization
in this case.
A related
consideration is some of the callback cases that currently don't
quite
work as I would expect, because injection can only happen when an
instance is created. I think there may be cases where we should be
injecting/binding a callback reference when the forward call is
received and disptached to an instance that already exists, and I'm
working on a use case description for this.
With Raymond's proposal, I think that the instances involved in
the call & callback are well known "in advance" and it should work
properly. Perhaps I'm wrong - and certainly it isn't the way it
works today, but it could be made to work that way.
Two stateless callbacks in a row would invoke two different
stateless instances. I think you need the proxy for these callback
cases also.
I would prefer to defer this change until after 1.0 so that we can
discuss it more fully to consider all the implications, and
coordinate
it with resolving the callback issues if we agree that a change is
needed there.
That's a fair point and I tend to agree, since this is an
optimisation, not some fundamental function. I suspect it will
take quite a piece of work to make it run properly in all scenarios.
Simon
I added the following test code to the Calculator sample:
public double add(double n1, double n2) {
//return addService.add(n1, n2);
// Warm up
double r = 0;
for (int i=0; i < 100; i++) {
r = addService.add(n1, n2);
}
// Perform 1 million calls
long n = 1000000;
long begin = System.currentTimeMillis();
for (int i=0; i < n; i++) {
r = addService.add(n1, n2);
}
long end = System.currentTimeMillis();
// Print the results
double time = ((double)(end - begin));
System.out.println("Calls: " + n);
System.out.println("Time: " + (long)time + " msec");
return r;
}
This code measures the time to perform 1 million invocations through the
CalculatorService/addService reference wired to the AddService component.
Here are some numbers, running in a single thread, on Sun JRE 1.5_10 -
Linux RHEL5 - Thinkpad T60P:
- without Raymond's patch, 1010 msec for 1 million invocations
- with Raymond's patch, 20 msec for 1 million invocations
==> a 50x performance optimization for a very specific wiring pattern,
but a very common one, two Java components with the same scope and no
interceptors between them.
I rebuilt the whole trunk with the patch and didn't get any regression.
I reviewed the code, the comments in this thread, and the spec
description of scopes.
My assessment is that it is safe to apply the patch, and desirable given
the performance improvement of 50x, with minor changes:
a) Test that the reference or service interface is conversational, and
in this case go the slow proxy route, as we need to intercept the
invocation to handle the conversation state. The fact that the build
didn't break with the patch tells me that we need one more test case for
conversations :)
b) In the new code test that scopeContainer is not null. It's not
supposed to be null on a JavaImplementation, but I'm paranoid :)
c) Scope.SYSTEM is not used, you don't need to test for it.
d) Testing the scope of the target is not sufficient, also test that the
scope of the source component is equals to the scope of the target.
Hope this helps.
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
