Avoid one Proxy creation when using injected mappers

[GoogleCodeExporter]

What version of the MyBatis are you using?
mybatis-spring 1.0.0-RC2

Please describe the problem.  Unit tests are best!
Right now. _Everytime_ a mapper method is called from a application bean, a new 
proxy is created by MyBatis and discarded. This may have a performance impact. 

I was thinking in a way to avoid the creation of that proxy and MyBatis 
SessionManager gave me the idea.

This change creates a SqlSessionTemplate based in a dynamic proxy over 
SqlSession intead of using callbacks. In fact, the proxy and the callbacks are 
two ways of achieving the same thing.

All test runs fine but one that checks that the connection is closed and it is 
not because it has not been opened and its initial status is closed=false

It is just fixed changing this method in AbstractMyBatisSpringTest

    @Before
    public void setupConnection() throws SQLException {
        connection = createMockConnection();
        connection.close(); // set that starts closed
        dataSource.setupConnection(connection);
    }

Advantages:
- cleaner design
- removed the SqlSessionCallback that is not easy to use
- avoid the creation of a new proxy on _each_ call to a mapper method thus 
improved performance.
- applications should use a SqlSession instead of a SqlSessionTemplate so we 
have less dependency on mybatis-spring. This will let changing to other 
container easily (i.e. Spring -> Guice). This can be also achieved with current 
impl making SqlSeesionTemplate implement SqlSession.

Disadvantages:
- SqlSessionTemplate must be constructed with a SqlSessionFactory that may 
break code that works with RC1 and RC2. Not a big problem.
- GA will be delayed :(
- Mybatis-spring integration will be a bit more different than iBATIS 2-spring 
integration.
- Manual must be changed.

Please have a look at the code and lets discuss it or on the dev ML.

Original issue reported on code.google.com by eduardo.macarron on 7 Nov 2010 at 6:45

Attachments:

• SqlSessionTemplate.java

[GoogleCodeExporter]

I'm +1 for that solution, IMHO is much better investing time on a better 
solution instead of hurry to publish a release. Even if RC2 works fine, GA will 
work better, BTW it's just my thought.
Anyone else?

Original comment by simone.t...@gmail.com on 7 Nov 2010 at 8:02

[GoogleCodeExporter]

I like this change. While the mini performance benchmarks I have run do not 
show that much a difference with the double proxy, not having it is always 
better in my opinion. It does make the code cleaner too. Also, we could drop 
SqlSessionOperations.

Do we still want SqlSessionCallback for instances when the user knows that 
multiple executions are needed in a single SqlSession (i.e. batch work). We 
could keep the execute(SqlSessionCallback) method just like it was in the older 
version of the template.

Thinking about this some more, if we make this change, we don't really even 
need the sqlSessionProxy! All that proxy is doing is the try..catch block. We 
could implement every method using the try..catch directly rather than a call 
through a proxy. It's not as pretty since we would have the same copy-paste 
code in every method, but it would be very fast.

Could we live with a little mess and needing more careful maintenance to get 
this benefit? I think so, but what does everyone else think?

Original comment by hpresnall@gmail.com on 7 Nov 2010 at 8:28

[GoogleCodeExporter]

I am afraid Hunter that we can not get rid of that proxy. If we get a 
SqlSession during bootstrap to build the mappers. Those mappers will hold a 
closed SqlSession when they are called in work threads. Removing it you get 
org.apache.ibatis.executor.ExecutorException: Executor was closed. We need a 
fake SqlSession that is able to route calls to the right one in runtime. I 
thought Simo had this problem in his Guice impl :) but SqlSessionManager is 
doing the trick for him.  

Regarding the callback I am not sure we need it because I think this code will 
work fine for batches if runned inside a tx.

    public User getUser(String userId) {
        SqlSession session = getSqlSessionTemplate();
        session.insert();
        session.insert();
    }

The most weird thing is setting the executor method. Guice can set Transaction 
attributes so the executor method is read from there. That is relly good, but 
we cannot do something like that (or at least I don't know enought about Spring 
tx) so this should be coded:

SqlSession s = new SqlSessionTemplate(sqlSessionFactory, ExecutorType.BATCH);
s.insert()
s.insert()
...

The point is that, as you already know, this cannot be mixed with other 
executor type method within the same transaction. 

Original comment by eduardo.macarron on 7 Nov 2010 at 9:03

[GoogleCodeExporter]

Yes, you are right about the proxy. I was missing the reference in the mapper.

That code will work for batches, but it's not optimal. Using an 
execute(SqlSessionCallback) would make sure the SqlSessionUtils code is only 
called once. It's a minor optimization, but I think it's still nice to have.

I don't see a way to avoid setting the ExecutorType and storing it in the 
template either. This seems like a good compromise since you can create a 
template with a different type than the default if you need too. Also, keeping 
the execute method would allow you to change this.

Original comment by hpresnall@gmail.com on 7 Nov 2010 at 9:51

[GoogleCodeExporter]

Yes, its true that SqlSessionUtils will be called on each statement but as 
SqlSession is already created we will just avoid this call
SqlSessionHolder holder = (SqlSessionHolder) 
TransactionSynchronizationManager.getResource(sessionFactory);
But that method seems to be quite light. It just gets a map with 2 elements 
from ThreadLocal and then gets the value from the map.

My concern is that there will be two ways to do the same thing and will be very 
difficult to explain to the end user which are the differences between them. I 
am not sure this is worth given the small optimization we will get. What do you 
think?

Original comment by eduardo.macarron on 7 Nov 2010 at 10:32

[GoogleCodeExporter]

There's some other overhead closing the session, but you are right that it's 
probably not enough to justify having a separate way to do things. We would 
have to come up with some rule for when to switch between standard usage and 
the callback. That's probably not possible for the general case.

Original comment by hpresnall@gmail.com on 7 Nov 2010 at 11:59

[GoogleCodeExporter]

Should SqlSessionTemplate.getConnection() call sqlSessionProxy.getConnection() 
to be consistent with DefaultSqlSession?

This would start an SqlSession if needed (via SqlSesisonUtils) and return the 
MyBatis Transaction's connection. By default, this will be the conn from 
SpringManagedTransaction which is one associated with the current Spring TX.

Original comment by hpresnall@gmail.com on 8 Nov 2010 at 12:09

[GoogleCodeExporter]

Yes hunter you are right. 

Change is commited so we call all review it evolve the code.

Original comment by eduardo.macarron on 8 Nov 2010 at 6:26

• Changed state: Fixed

[GoogleCodeExporter]

Original comment by eduardo.macarron on 30 Dec 2010 at 9:23

• Added labels: Target-Release1.0.0RC3