Fist of all happy new year!

Some work has been doing on Spring 3 and Ibatis 3 integration.

Having a look at mappers internals we saw that a new MapperMethod is created (with some introspection work) on each call to a mapper method. I wonder if it would be better to build them all during startup or maybe cache them somehow.

this is the Jira issue (http://jira.springframework.org/browse/SPR-5991) Grabriel Axel posted it here some months ago (he opened the issue)

The main problem with this task is that MapperMethods are not thread safe and that they hold an SqlSession. Enabling cache would require to make some changes to MapperMethods. SqlSession should be passed as an argument to execute instead to its constructor. 

 public MapperMethod(Method method, Configuration configuration) {
    paramNames = new ArrayList<String>();
    paramPositions = new ArrayList<Integer>();
    ...

  public Object execute(Object[] args, SqlSession sqlSession) {
    Object result;
    if (SqlCommandType.INSERT == type) {
    ...

And MapperProxy

  public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
    return new MapperMethod(method, sqlSession.getConfiguration()).execute(args, sqlSession);
  }


So for example Spring could have this code to use directly injected Mappers where SqlSession is got from spring's transaction context.

private Map<Method, MapperMethod> mapperMethods = new ConcurrentHashMap<Method, MapperMethod>();

public <T> T getMapper(final Class<T> type) {
    // mimic iBATIS MapperProxy
    return (T) java.lang.reflect.Proxy.newProxyInstance(type.getClassLoader(), new Class[] { type },
            new InvocationHandler() {
                <at> Override
                public Object invoke(final Object proxy, final Method method, final Object[] args) throws Throwable {
                    return execute(new SqlSessionCallback<T>() {
                        <at> Override
                        public T doInSqlSession(SqlSession sqlSession) throws SQLException {
                               // mimic iBATIS MapperProxy
                            Class<?> type = method.getDeclaringClass();
                                if (!sqlSession.getConfiguration().hasMapper(type)) {
                                throw new BindingException("Type " + type + " is not known to the MapperRegistry.");
                            }

                            MapperMethod mm = mapperMethods.get(method);
                            if (mm == null) {
                                    mm = new MapperMethod(method, sqlSession.getConfiguration());
                                    mapperMethods.put(method, mm);
                                } else {
                                    logger.debug("Returning a cached mapper method");
                                }
                               
                                return (T) mm.execute(args, sqlSession);
                            }
                        });
                    }
                });
    }

Or maybe avoid using directly MapperMethods and access to them thought the "standard" api SqlSession.getConfiguration().getMapper(impl, sesion) but in that case caching should be done inside.

what do you think about this?

What problem are you trying to solve, or goal you're trying to achieve by doing this?  Is it strictly a performance consideration? 

Clinton

On Fri, Jan 1, 2010 at 7:31 AM, Eduardo Macarron <eduardo.macarron <at> gmail.com> wrote:

Fist of all happy new year!

Some work has been doing on Spring 3 and Ibatis 3 integration.

Having a look at mappers internals we saw that a new MapperMethod is created (with some introspection work) on each call to a mapper method. I wonder if it would be better to build them all during startup or maybe cache them somehow.

this is the Jira issue (http://jira.springframework.org/browse/SPR-5991) Grabriel Axel posted it here some months ago (he opened the issue)

The main problem with this task is that MapperMethods are not thread safe and that they hold an SqlSession. Enabling cache would require to make some changes to MapperMethods. SqlSession should be passed as an argument to execute instead to its constructor. 

 public MapperMethod(Method method, Configuration configuration) {
    paramNames = new ArrayList<String>();
    paramPositions = new ArrayList<Integer>();
    ...

  public Object execute(Object[] args, SqlSession sqlSession) {
    Object result;
    if (SqlCommandType.INSERT == type) {
    ...

And MapperProxy

  public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
    return new MapperMethod(method, sqlSession.getConfiguration()).execute(args, sqlSession);
  }


So for example Spring could have this code to use directly injected Mappers where SqlSession is got from spring's transaction context.

private Map<Method, MapperMethod> mapperMethods = new ConcurrentHashMap<Method, MapperMethod>();

public <T> T getMapper(final Class<T> type) {
    // mimic iBATIS MapperProxy
    return (T) java.lang.reflect.Proxy.newProxyInstance(type.getClassLoader(), new Class[] { type },
            new InvocationHandler() {
                <at> Override
                public Object invoke(final Object proxy, final Method method, final Object[] args) throws Throwable {
                    return execute(new SqlSessionCallback<T>() {
                        <at> Override
                        public T doInSqlSession(SqlSession sqlSession) throws SQLException {
                               // mimic iBATIS MapperProxy
                            Class<?> type = method.getDeclaringClass();
                                if (!sqlSession.getConfiguration().hasMapper(type)) {
                                throw new BindingException("Type " + type + " is not known to the MapperRegistry.");
                            }

                            MapperMethod mm = mapperMethods.get(method);
                            if (mm == null) {
                                    mm = new MapperMethod(method, sqlSession.getConfiguration());
                                    mapperMethods.put(method, mm);
                                } else {
                                    logger.debug("Returning a cached mapper method");
                                }
                               
                                return (T) mm.execute(args, sqlSession);
                            }
                        });
                    }
                });
    }

Or maybe avoid using directly MapperMethods and access to them thought the "standard" api SqlSession.getConfiguration().getMapper(impl, sesion) but in that case caching should be done inside.

what do you think about this?

Yes Clinton, is just for performance.

2010/1/1 Clinton Begin <clinton.begin <at> gmail.com>

What problem are you trying to solve, or goal you're trying to achieve by doing this?  Is it strictly a performance consideration? 

Clinton

On Fri, Jan 1, 2010 at 7:31 AM, Eduardo Macarron <eduardo.macarron <at> gmail.com> wrote:

Fist of all happy new year!

Some work has been doing on Spring 3 and Ibatis 3 integration.

Having a look at mappers internals we saw that a new MapperMethod is created (with some introspection work) on each call to a mapper method. I wonder if it would be better to build them all during startup or maybe cache them somehow.

this is the Jira issue (http://jira.springframework.org/browse/SPR-5991) Grabriel Axel posted it here some months ago (he opened the issue)

The main problem with this task is that MapperMethods are not thread safe and that they hold an SqlSession. Enabling cache would require to make some changes to MapperMethods. SqlSession should be passed as an argument to execute instead to its constructor. 

 public MapperMethod(Method method, Configuration configuration) {
    paramNames = new ArrayList<String>();
    paramPositions = new ArrayList<Integer>();
    ...

  public Object execute(Object[] args, SqlSession sqlSession) {
    Object result;
    if (SqlCommandType.INSERT == type) {
    ...

And MapperProxy

  public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
    return new MapperMethod(method, sqlSession.getConfiguration()).execute(args, sqlSession);
  }


So for example Spring could have this code to use directly injected Mappers where SqlSession is got from spring's transaction context.

private Map<Method, MapperMethod> mapperMethods = new ConcurrentHashMap<Method, MapperMethod>();

public <T> T getMapper(final Class<T> type) {
    // mimic iBATIS MapperProxy
    return (T) java.lang.reflect.Proxy.newProxyInstance(type.getClassLoader(), new Class[] { type },
            new InvocationHandler() {
                <at> Override
                public Object invoke(final Object proxy, final Method method, final Object[] args) throws Throwable {
                    return execute(new SqlSessionCallback<T>() {
                        <at> Override
                        public T doInSqlSession(SqlSession sqlSession) throws SQLException {
                               // mimic iBATIS MapperProxy
                            Class<?> type = method.getDeclaringClass();
                                if (!sqlSession.getConfiguration().hasMapper(type)) {
                                throw new BindingException("Type " + type + " is not known to the MapperRegistry.");
                            }

                            MapperMethod mm = mapperMethods.get(method);
                            if (mm == null) {
                                    mm = new MapperMethod(method, sqlSession.getConfiguration());
                                    mapperMethods.put(method, mm);
                                } else {
                                    logger.debug("Returning a cached mapper method");
                                }
                               
                                return (T) mm.execute(args, sqlSession);
                            }
                        });
                    }
                });
    }

Or maybe avoid using directly MapperMethods and access to them thought the "standard" api SqlSession.getConfiguration().getMapper(impl, sesion) but in that case caching should be done inside.

what do you think about this?

Do you have some profiler metrics or benchmarks to support the concern?  Even completely unoptimized, I'd be surprised if this presented any sort of performance challenge on a modern VM. 

While the MapperMethod may look slow when you read the code, realize that most of the for loops will execute either 0 or 1 times.  Or maybe a couple of times for methods that have multiple parameters or the odd one that has multiple annotations.  Even then, it's 1 - 4 times against fairly quick code.  The biggest performance impact is probably some of the string conversion and concatenation, which could probably be optimized. 

More important than the micro-optimizations though, is that MapperMethod is only ever called on the outermost call from the consumer of a Mapper interface.  So it's a very high level call... just enough to marshal it down to a sqlSession.[select,insert,update,delete] call. 

Even at 40 TPS (a good benchmark for 1,000,000 concurrent requests in an 8 hour period) you're looking at literally 40 * 4 operations per second at most.  I wouldn't be overly concerned with those calls.  Even the potential creation and destruction of maybe a few hundred string objects under that amount of load, is nothing more than light exercise for the garbage collector.

I think there may be room for improvement, but not enough to warrant introducing state into a stateless design. 

If you have some interesting profiler metrics that show a potential bottleneck, I'd be very interested in seeing it. 

Cheers,
Clinton

On Fri, Jan 1, 2010 at 9:17 AM, Eduardo Macarron <eduardo.macarron <at> gmail.com> wrote:

Yes Clinton, is just for performance.

2010/1/1 Clinton Begin <clinton.begin <at> gmail.com>

What problem are you trying to solve, or goal you're trying to achieve by doing this?  Is it strictly a performance consideration? 

Clinton

On Fri, Jan 1, 2010 at 7:31 AM, Eduardo Macarron <eduardo.macarron <at> gmail.com> wrote:

Fist of all happy new year!

Some work has been doing on Spring 3 and Ibatis 3 integration.

Having a look at mappers internals we saw that a new MapperMethod is created (with some introspection work) on each call to a mapper method. I wonder if it would be better to build them all during startup or maybe cache them somehow.

this is the Jira issue (http://jira.springframework.org/browse/SPR-5991) Grabriel Axel posted it here some months ago (he opened the issue)

The main problem with this task is that MapperMethods are not thread safe and that they hold an SqlSession. Enabling cache would require to make some changes to MapperMethods. SqlSession should be passed as an argument to execute instead to its constructor. 

 public MapperMethod(Method method, Configuration configuration) {
    paramNames = new ArrayList<String>();
    paramPositions = new ArrayList<Integer>();
    ...

  public Object execute(Object[] args, SqlSession sqlSession) {
    Object result;
    if (SqlCommandType.INSERT == type) {
    ...

And MapperProxy

  public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
    return new MapperMethod(method, sqlSession.getConfiguration()).execute(args, sqlSession);
  }


So for example Spring could have this code to use directly injected Mappers where SqlSession is got from spring's transaction context.

private Map<Method, MapperMethod> mapperMethods = new ConcurrentHashMap<Method, MapperMethod>();

public <T> T getMapper(final Class<T> type) {
    // mimic iBATIS MapperProxy
    return (T) java.lang.reflect.Proxy.newProxyInstance(type.getClassLoader(), new Class[] { type },
            new InvocationHandler() {
                <at> Override
                public Object invoke(final Object proxy, final Method method, final Object[] args) throws Throwable {
                    return execute(new SqlSessionCallback<T>() {
                        <at> Override
                        public T doInSqlSession(SqlSession sqlSession) throws SQLException {
                               // mimic iBATIS MapperProxy
                            Class<?> type = method.getDeclaringClass();
                                if (!sqlSession.getConfiguration().hasMapper(type)) {
                                throw new BindingException("Type " + type + " is not known to the MapperRegistry.");
                            }

                            MapperMethod mm = mapperMethods.get(method);
                            if (mm == null) {
                                    mm = new MapperMethod(method, sqlSession.getConfiguration());
                                    mapperMethods.put(method, mm);
                                } else {
                                    logger.debug("Returning a cached mapper method");
                                }
                               
                                return (T) mm.execute(args, sqlSession);
                            }
                        });
                    }
                });
    }

Or maybe avoid using directly MapperMethods and access to them thought the "standard" api SqlSession.getConfiguration().getMapper(impl, sesion) but in that case caching should be done inside.

what do you think about this?

Clinton, you are completely right. On current design, and doing some very basic measures, caching MapperMethods does not provide a significant performance improvement. In my laptop, building a MapperMethod for a simple method requires about 13 microseconds and getting if from a map is about 4 microseconds. So as I said caching does not make a big difference.

I wanted just to point that current design disables caching because MapperMethods depend on sessions. That dependency seems unnecesary and removing it with what seems a simple change may open the way to caching.

I don't know if MapperMethod creation will be heavier in future versions. For example climbing up in interface hierarchy, filtering java.lang.Object method calls like toString(), performing more checks or maybe other features yet to come. In fact some "caching" is already been done because mappers methods are procesed to search statement annotations during startup.

Anyway I am aware of the current status of the project and also that any change should have a good reason to be done.

2010/1/1 Clinton Begin <clinton.begin <at> gmail.com>

Do you have some profiler metrics or benchmarks to support the concern?  Even completely unoptimized, I'd be surprised if this presented any sort of performance challenge on a modern VM. 

While the MapperMethod may look slow when you read the code, realize that most of the for loops will execute either 0 or 1 times.  Or maybe a couple of times for methods that have multiple parameters or the odd one that has multiple annotations.  Even then, it's 1 - 4 times against fairly quick code.  The biggest performance impact is probably some of the string conversion and concatenation, which could probably be optimized. 

More important than the micro-optimizations though, is that MapperMethod is only ever called on the outermost call from the consumer of a Mapper interface.  So it's a very high level call... just enough to marshal it down to a sqlSession.[select,insert,update,delete] call. 

Even at 40 TPS (a good benchmark for 1,000,000 concurrent requests in an 8 hour period) you're looking at literally 40 * 4 operations per second at most.  I wouldn't be overly concerned with those calls.  Even the potential creation and destruction of maybe a few hundred string objects under that amount of load, is nothing more than light exercise for the garbage collector.

I think there may be room for improvement, but not enough to warrant introducing state into a stateless design. 

If you have some interesting profiler metrics that show a potential bottleneck, I'd be very interested in seeing it. 

Cheers,
Clinton

On Fri, Jan 1, 2010 at 9:17 AM, Eduardo Macarron <eduardo.macarron <at> gmail.com> wrote:

Yes Clinton, is just for performance.

2010/1/1 Clinton Begin <clinton.begin <at> gmail.com>

What problem are you trying to solve, or goal you're trying to achieve by doing this?  Is it strictly a performance consideration? 

Clinton

On Fri, Jan 1, 2010 at 7:31 AM, Eduardo Macarron <eduardo.macarron <at> gmail.com> wrote:

Fist of all happy new year!

Some work has been doing on Spring 3 and Ibatis 3 integration.

Having a look at mappers internals we saw that a new MapperMethod is created (with some introspection work) on each call to a mapper method. I wonder if it would be better to build them all during startup or maybe cache them somehow.

this is the Jira issue (http://jira.springframework.org/browse/SPR-5991) Grabriel Axel posted it here some months ago (he opened the issue)

The main problem with this task is that MapperMethods are not thread safe and that they hold an SqlSession. Enabling cache would require to make some changes to MapperMethods. SqlSession should be passed as an argument to execute instead to its constructor. 

 public MapperMethod(Method method, Configuration configuration) {
    paramNames = new ArrayList<String>();
    paramPositions = new ArrayList<Integer>();
    ...

  public Object execute(Object[] args, SqlSession sqlSession) {
    Object result;
    if (SqlCommandType.INSERT == type) {
    ...

And MapperProxy

  public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
    return new MapperMethod(method, sqlSession.getConfiguration()).execute(args, sqlSession);
  }


So for example Spring could have this code to use directly injected Mappers where SqlSession is got from spring's transaction context.

private Map<Method, MapperMethod> mapperMethods = new ConcurrentHashMap<Method, MapperMethod>();

public <T> T getMapper(final Class<T> type) {
    // mimic iBATIS MapperProxy
    return (T) java.lang.reflect.Proxy.newProxyInstance(type.getClassLoader(), new Class[] { type },
            new InvocationHandler() {
                <at> Override
                public Object invoke(final Object proxy, final Method method, final Object[] args) throws Throwable {
                    return execute(new SqlSessionCallback<T>() {
                        <at> Override
                        public T doInSqlSession(SqlSession sqlSession) throws SQLException {
                               // mimic iBATIS MapperProxy
                            Class<?> type = method.getDeclaringClass();
                                if (!sqlSession.getConfiguration().hasMapper(type)) {
                                throw new BindingException("Type " + type + " is not known to the MapperRegistry.");
                            }

                            MapperMethod mm = mapperMethods.get(method);
                            if (mm == null) {
                                    mm = new MapperMethod(method, sqlSession.getConfiguration());
                                    mapperMethods.put(method, mm);
                                } else {
                                    logger.debug("Returning a cached mapper method");
                                }
                               
                                return (T) mm.execute(args, sqlSession);
                            }
                        });
                    }
                });
    }

Or maybe avoid using directly MapperMethods and access to them thought the "standard" api SqlSession.getConfiguration().getMapper(impl, sesion) but in that case caching should be done inside.

what do you think about this?

Thanks Eduardo.  I'm open to design suggestions, but will be judicious about performance related refactorings or anything that introduces state into the execution engine.

Here's a bit of design background:

When you read down the package structure of the org.apache.ibatis, generally all state is ultimately stored within the Configuration class and the various objects that it collects and manages.  Most of these are in the mapping and cache packages.  The entire execution package (the 'execution engine') is stateless (perhaps with one or two exceptions that I cannot even  recall at the moment).  Every other package contains various supporting utility classes.

So between the mapping package and the execution package, you have 99% of what's important to iBATIS.  The mapping package is the state, largely dumb state.  The execution package is the stateless and contains all of the behavior and logic. 

This separation is important to keeping the design as simple as possible.  iBATIS 1 used a "smart object" design, where all state and behavior was co-located, that was inflexible and messy.  iBATIS 2 used a stateful execution pipeline that was very fast, but extremely complicated.  Few could understand that code.  iBATIS 3 focuses heavily on code that's both flexible and easier to read, possibly at the cost of a bit of performance, until it becomes a problem.

I've been really happy with this focus, as your suggestions are proof that the code is succeeding (at least better than its predecessors) in that you're able to read it, follow along, figure it out and make suggestions.

They are most welcome, even if we don't accept every one. 

Cheers,
Clinton

On Fri, Jan 1, 2010 at 1:57 PM, Eduardo Macarron <eduardo.macarron <at> gmail.com> wrote:

Clinton, you are completely right. On current design, and doing some very basic measures, caching MapperMethods does not provide a significant performance improvement. In my laptop, building a MapperMethod for a simple method requires about 13 microseconds and getting if from a map is about 4 microseconds. So as I said caching does not make a big difference.

I wanted just to point that current design disables caching because MapperMethods depend on sessions. That dependency seems unnecesary and removing it with what seems a simple change may open the way to caching.

I don't know if MapperMethod creation will be heavier in future versions. For example climbing up in interface hierarchy, filtering java.lang.Object method calls like toString(), performing more checks or maybe other features yet to come. In fact some "caching" is already been done because mappers methods are procesed to search statement annotations during startup.

Anyway I am aware of the current status of the project and also that any change should have a good reason to be done.

2010/1/1 Clinton Begin <clinton.begin <at> gmail.com>

Do you have some profiler metrics or benchmarks to support the concern?  Even completely unoptimized, I'd be surprised if this presented any sort of performance challenge on a modern VM. 

While the MapperMethod may look slow when you read the code, realize that most of the for loops will execute either 0 or 1 times.  Or maybe a couple of times for methods that have multiple parameters or the odd one that has multiple annotations.  Even then, it's 1 - 4 times against fairly quick code.  The biggest performance impact is probably some of the string conversion and concatenation, which could probably be optimized. 

More important than the micro-optimizations though, is that MapperMethod is only ever called on the outermost call from the consumer of a Mapper interface.  So it's a very high level call... just enough to marshal it down to a sqlSession.[select,insert,update,delete] call. 

Even at 40 TPS (a good benchmark for 1,000,000 concurrent requests in an 8 hour period) you're looking at literally 40 * 4 operations per second at most.  I wouldn't be overly concerned with those calls.  Even the potential creation and destruction of maybe a few hundred string objects under that amount of load, is nothing more than light exercise for the garbage collector.

I think there may be room for improvement, but not enough to warrant introducing state into a stateless design. 

If you have some interesting profiler metrics that show a potential bottleneck, I'd be very interested in seeing it. 

Cheers,
Clinton

On Fri, Jan 1, 2010 at 9:17 AM, Eduardo Macarron <eduardo.macarron <at> gmail.com> wrote:

Yes Clinton, is just for performance.

2010/1/1 Clinton Begin <clinton.begin <at> gmail.com>

What problem are you trying to solve, or goal you're trying to achieve by doing this?  Is it strictly a performance consideration? 

Clinton

On Fri, Jan 1, 2010 at 7:31 AM, Eduardo Macarron <eduardo.macarron <at> gmail.com> wrote:

Fist of all happy new year!

Some work has been doing on Spring 3 and Ibatis 3 integration.

Having a look at mappers internals we saw that a new MapperMethod is created (with some introspection work) on each call to a mapper method. I wonder if it would be better to build them all during startup or maybe cache them somehow.

this is the Jira issue (http://jira.springframework.org/browse/SPR-5991) Grabriel Axel posted it here some months ago (he opened the issue)

The main problem with this task is that MapperMethods are not thread safe and that they hold an SqlSession. Enabling cache would require to make some changes to MapperMethods. SqlSession should be passed as an argument to execute instead to its constructor. 

 public MapperMethod(Method method, Configuration configuration) {
    paramNames = new ArrayList<String>();
    paramPositions = new ArrayList<Integer>();
    ...

  public Object execute(Object[] args, SqlSession sqlSession) {
    Object result;
    if (SqlCommandType.INSERT == type) {
    ...

And MapperProxy

  public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
    return new MapperMethod(method, sqlSession.getConfiguration()).execute(args, sqlSession);
  }


So for example Spring could have this code to use directly injected Mappers where SqlSession is got from spring's transaction context.

private Map<Method, MapperMethod> mapperMethods = new ConcurrentHashMap<Method, MapperMethod>();

public <T> T getMapper(final Class<T> type) {
    // mimic iBATIS MapperProxy
    return (T) java.lang.reflect.Proxy.newProxyInstance(type.getClassLoader(), new Class[] { type },
            new InvocationHandler() {
                <at> Override
                public Object invoke(final Object proxy, final Method method, final Object[] args) throws Throwable {
                    return execute(new SqlSessionCallback<T>() {
                        <at> Override
                        public T doInSqlSession(SqlSession sqlSession) throws SQLException {
                               // mimic iBATIS MapperProxy
                            Class<?> type = method.getDeclaringClass();
                                if (!sqlSession.getConfiguration().hasMapper(type)) {
                                throw new BindingException("Type " + type + " is not known to the MapperRegistry.");
                            }

                            MapperMethod mm = mapperMethods.get(method);
                            if (mm == null) {
                                    mm = new MapperMethod(method, sqlSession.getConfiguration());
                                    mapperMethods.put(method, mm);
                                } else {
                                    logger.debug("Returning a cached mapper method");
                                }
                               
                                return (T) mm.execute(args, sqlSession);
                            }
                        });
                    }
                });
    }

Or maybe avoid using directly MapperMethods and access to them thought the "standard" api SqlSession.getConfiguration().getMapper(impl, sesion) but in that case caching should be done inside.

what do you think about this?

Thanks for the explanation Clinton.

But let me just point that I don't think my suggested change breaks this design principles.

When Configuration registers a new Mapper record on MapperRegistry, MapperRegistry could create MapperMethods, hold them on a Map (maybe instead of the Set it uses now), and pass this Map to MapperProxy so that it simply uses its content and does not create any new Object.

Regardless I got to all this while digging into internals for integrating Ibatis with Spring, trying to cache this MapperMethods at Spring layer, and finding that... it was not possible! But maybe we'd better go back and propose an implementation much more compilant to Ibatis api, forget about MapperMethods and let Ibatis handle them in the way it prefers.

Thanks again for your time and for your patience. Hope that GA comes soon!!

2010/1/1 Clinton Begin <clinton.begin <at> gmail.com>

Thanks Eduardo.  I'm open to design suggestions, but will be judicious about performance related refactorings or anything that introduces state into the execution engine.

Here's a bit of design background:

When you read down the package structure of the org.apache.ibatis, generally all state is ultimately stored within the Configuration class and the various objects that it collects and manages.  Most of these are in the mapping and cache packages.  The entire execution package (the 'execution engine') is stateless (perhaps with one or two exceptions that I cannot even  recall at the moment).  Every other package contains various supporting utility classes.

So between the mapping package and the execution package, you have 99% of what's important to iBATIS.  The mapping package is the state, largely dumb state.  The execution package is the stateless and contains all of the behavior and logic. 

This separation is important to keeping the design as simple as possible.  iBATIS 1 used a "smart object" design, where all state and behavior was co-located, that was inflexible and messy.  iBATIS 2 used a stateful execution pipeline that was very fast, but extremely complicated.  Few could understand that code.  iBATIS 3 focuses heavily on code that's both flexible and easier to read, possibly at the cost of a bit of performance, until it becomes a problem.

I've been really happy with this focus, as your suggestions are proof that the code is succeeding (at least better than its predecessors) in that you're able to read it, follow along, figure it out and make suggestions.

They are most welcome, even if we don't accept every one. 

Cheers,
Clinton

On Fri, Jan 1, 2010 at 1:57 PM, Eduardo Macarron <eduardo.macarron <at> gmail.com> wrote:

Clinton, you are completely right. On current design, and doing some very basic measures, caching MapperMethods does not provide a significant performance improvement. In my laptop, building a MapperMethod for a simple method requires about 13 microseconds and getting if from a map is about 4 microseconds. So as I said caching does not make a big difference.

I wanted just to point that current design disables caching because MapperMethods depend on sessions. That dependency seems unnecesary and removing it with what seems a simple change may open the way to caching.

I don't know if MapperMethod creation will be heavier in future versions. For example climbing up in interface hierarchy, filtering java.lang.Object method calls like toString(), performing more checks or maybe other features yet to come. In fact some "caching" is already been done because mappers methods are procesed to search statement annotations during startup.

Anyway I am aware of the current status of the project and also that any change should have a good reason to be done.

2010/1/1 Clinton Begin <clinton.begin <at> gmail.com>

Do you have some profiler metrics or benchmarks to support the concern?  Even completely unoptimized, I'd be surprised if this presented any sort of performance challenge on a modern VM. 

While the MapperMethod may look slow when you read the code, realize that most of the for loops will execute either 0 or 1 times.  Or maybe a couple of times for methods that have multiple parameters or the odd one that has multiple annotations.  Even then, it's 1 - 4 times against fairly quick code.  The biggest performance impact is probably some of the string conversion and concatenation, which could probably be optimized. 

More important than the micro-optimizations though, is that MapperMethod is only ever called on the outermost call from the consumer of a Mapper interface.  So it's a very high level call... just enough to marshal it down to a sqlSession.[select,insert,update,delete] call. 

Even at 40 TPS (a good benchmark for 1,000,000 concurrent requests in an 8 hour period) you're looking at literally 40 * 4 operations per second at most.  I wouldn't be overly concerned with those calls.  Even the potential creation and destruction of maybe a few hundred string objects under that amount of load, is nothing more than light exercise for the garbage collector.

I think there may be room for improvement, but not enough to warrant introducing state into a stateless design. 

If you have some interesting profiler metrics that show a potential bottleneck, I'd be very interested in seeing it. 

Cheers,
Clinton

On Fri, Jan 1, 2010 at 9:17 AM, Eduardo Macarron <eduardo.macarron <at> gmail.com> wrote:

Yes Clinton, is just for performance.

2010/1/1 Clinton Begin <clinton.begin <at> gmail.com>

What problem are you trying to solve, or goal you're trying to achieve by doing this?  Is it strictly a performance consideration? 

Clinton

On Fri, Jan 1, 2010 at 7:31 AM, Eduardo Macarron <eduardo.macarron <at> gmail.com> wrote:

Fist of all happy new year!

Some work has been doing on Spring 3 and Ibatis 3 integration.

Having a look at mappers internals we saw that a new MapperMethod is created (with some introspection work) on each call to a mapper method. I wonder if it would be better to build them all during startup or maybe cache them somehow.

this is the Jira issue (http://jira.springframework.org/browse/SPR-5991) Grabriel Axel posted it here some months ago (he opened the issue)

The main problem with this task is that MapperMethods are not thread safe and that they hold an SqlSession. Enabling cache would require to make some changes to MapperMethods. SqlSession should be passed as an argument to execute instead to its constructor. 

 public MapperMethod(Method method, Configuration configuration) {
    paramNames = new ArrayList<String>();
    paramPositions = new ArrayList<Integer>();
    ...

  public Object execute(Object[] args, SqlSession sqlSession) {
    Object result;
    if (SqlCommandType.INSERT == type) {
    ...

And MapperProxy

  public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
    return new MapperMethod(method, sqlSession.getConfiguration()).execute(args, sqlSession);
  }


So for example Spring could have this code to use directly injected Mappers where SqlSession is got from spring's transaction context.

private Map<Method, MapperMethod> mapperMethods = new ConcurrentHashMap<Method, MapperMethod>();

public <T> T getMapper(final Class<T> type) {
    // mimic iBATIS MapperProxy
    return (T) java.lang.reflect.Proxy.newProxyInstance(type.getClassLoader(), new Class[] { type },
            new InvocationHandler() {
                <at> Override
                public Object invoke(final Object proxy, final Method method, final Object[] args) throws Throwable {
                    return execute(new SqlSessionCallback<T>() {
                        <at> Override
                        public T doInSqlSession(SqlSession sqlSession) throws SQLException {
                               // mimic iBATIS MapperProxy
                            Class<?> type = method.getDeclaringClass();
                                if (!sqlSession.getConfiguration().hasMapper(type)) {
                                throw new BindingException("Type " + type + " is not known to the MapperRegistry.");
                            }

                            MapperMethod mm = mapperMethods.get(method);
                            if (mm == null) {
                                    mm = new MapperMethod(method, sqlSession.getConfiguration());
                                    mapperMethods.put(method, mm);
                                } else {
                                    logger.debug("Returning a cached mapper method");
                                }
                               
                                return (T) mm.execute(args, sqlSession);
                            }
                        });
                    }
                });
    }

Or maybe avoid using directly MapperMethods and access to them thought the "standard" api SqlSession.getConfiguration().getMapper(impl, sesion) but in that case caching should be done inside.

what do you think about this?