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.macar...@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.be...@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.macar...@gmail.com> wrote: >> >>> Yes Clinton, is just for performance. >>> >>> >>> 2010/1/1 Clinton Begin <clinton.be...@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.macar...@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() { >>>>> @Override >>>>> public Object invoke(final Object proxy, final Method >>>>> method, final Object[] args) throws Throwable { >>>>> return execute(new SqlSessionCallback<T>() { >>>>> @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? >>>>> >>>>> >>>>> >>>> >>> >> >
