ibatis-user-java mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Eduardo Macarron <eduardo.macar...@gmail.com>
Subject Re: MapperMethod caching?
Date Sat, 02 Jan 2010 00:55:24 GMT
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@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@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@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@gmail.com> wrote:
>>>> Yes Clinton, is just for performance.
>>>> 2010/1/1 Clinton Begin <clinton.begin@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@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
>>>>>> created (with some introspection work) on each call to a mapper method.
>>>>>> 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
>>>>>> { 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?

View raw message