Use of reflection

[Rickard Öberg <ri...@xpedio.com>]

Hey

Browsing through the Velocity code I noticed that you use reflection
quite a lot, which is good, but that not much is cached, which is bad.

There are two operations in reflection:
1. Looking up methods. This is slow
2. Invoking methods. This is fast

Because of this you really really want to cache the method lookups. We
do this in the JBoss EJB server, and it helps performance a lot.

I am mostly thinking about ASTReference where reflection lookups are
made. Simply cache the result and you should see some performance
improvements.

The caching could be extremely easy to implement due to the heuristic
that a particular node in the AST will most likely see the same class on
each run (IMO the case that the same class is not seen will be *very*
uncommon). The first time you look it up simply store the class and
method in class attributes "cachedClass" and "cachedMethod". Then,
insert this if-statement before the method lookup:
		if (cachedClass != null && cachedClass == c)
		{
			m = cachedMethod;
		}
		else
		{
                try
                {
                    m = c.getMethod("set" + identifier, params);
		    cachedClass = c;
		    cachedMethod = m;
                }
              .....
This is not threadsafe, but that doesn't really matter in this case. It
also allows class reloading since the "cachedClass == c" test will
simply fail when the class has been updated.

I tried adding this fix to the Velocity sources (mainly ASTReference)
and it did improve performance somewhat when running the monthlist.vm
test page mentioned earlier.

Looking through the Velocity sources some more I find that there are
some caching done by using maps and such (Introspector). I would replace
that with this simpler approach, since the overhead of the above
if-statement is much less than using caches implemented as global maps
which is almost as heavy as using reflection as it is.

HTH,
  Rickard

-- 
Rickard Öberg
Software Development Specialist
xlurc - Xpedio Linköping Ubiquitous Research Center
Author of "Mastering RMI"
Email: rickard@xpedio.com

Re: Use of reflection

["Geir Magnusson Jr." <ge...@optonline.net>]

Rickard Öberg wrote:
> 
> Hey
> 
> Browsing through the Velocity code I noticed that you use reflection
> quite a lot, which is good, but that not much is cached, which is bad.
>

Where is it not cached?

> There are two operations in reflection:
> 1. Looking up methods. This is slow
> 2. Invoking methods. This is fast
> 
> Because of this you really really want to cache the method lookups. We
> do this in the JBoss EJB server, and it helps performance a lot.
> 
> I am mostly thinking about ASTReference where reflection lookups are
> made. Simply cache the result and you should see some performance
> improvements.

There is a spot in there, where we could do something, however, you have
to be careful - there is *nothing* that implies that the class backing
$foo in the first rendering of a template is the same as the next time
something goes through.

At first, Velocity *did* cache class/method info right in the nodes, and
it was a problem.  We found that the change, to the current two-level
cache system we have now, made little performance difference in general.
 
> The caching could be extremely easy to implement due to the heuristic
> that a particular node in the AST will most likely see the same class on
> each run (IMO the case that the same class is not seen will be *very*
> uncommon). The first time you look it up simply store the class and
> method in class attributes "cachedClass" and "cachedMethod". Then,
> insert this if-statement before the method lookup:
>                 if (cachedClass != null && cachedClass == c)
>                 {
>                         m = cachedMethod;
>                 }
>                 else
>                 {
>                 try
>                 {
>                     m = c.getMethod("set" + identifier, params);
>                     cachedClass = c;
>                     cachedMethod = m;
>                 }
>               .....
> This is not threadsafe, but that doesn't really matter in this case. It
> also allows class reloading since the "cachedClass == c" test will
> simply fail when the class has been updated.

Ah - good you saw the issue.

But threadsafety is important.

> I tried adding this fix to the Velocity sources (mainly ASTReference)
> and it did improve performance somewhat when running the monthlist.vm
> test page mentioned earlier.

I don't think it will be a big boost.
 
> Looking through the Velocity sources some more I find that there are
> some caching done by using maps and such (Introspector). I would replace
> that with this simpler approach, since the overhead of the above
> if-statement is much less than using caches implemented as global maps
> which is almost as heavy as using reflection as it is.

Like I said, it was the other way before, it was switched, and there was
little penalty.  Some, but the threadsafety and 'introspective
polymorphism' is worth it, I think.

geir

-- 
Geir Magnusson Jr.                           geirm@optonline.net
System and Software Consulting
Developing for the web?  See http://jakarta.apache.org/velocity/
Be well, do good work, and keep in touch.