db-ojb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lance Eason" <lance.ea...@whisperwire.com>
Subject Some performance tuning changes
Date Mon, 12 May 2003 06:00:27 GMT
I've been profiling my system with some large queries (returning thousands of items each).
 As a result I ended up making a few code changes to tune OJB.  All told they improve OJB's
performance by about 20% and reduce object creation by about the same for the scenarios I've
tested, nothing earth-shattering but every bit helps.  Only two of the changes required any
interface changes, but one of those is also the single most significant change.

Could some committer review and add them?  All changes are made against the latest from CVS
as of this afternoon.  Because of mailer limitations I'm sending the files as a follow-up
to this message.

The changes I made, from most significant to least significant:

1) RowReaderDefaultImpl retrieves values from the result set via column name instead of column
index.  This quickly showed up as a hotspot in the profiler.  While it's definitely a lot
more robust to use the name instead of the index there's no reason to pay the overhead of
the name lookup (which is significant) on each and every row.  The names can be converted
to indices once up front using ResultSet.findColumn(name) and then values can be retrieved
by index for each row.  Since the same RowReader may service multiple RsIterators at the same
time and different RsIterators may have different indices for the same column, the indices
have to be local to the RsIterator somehow.  I went with a sort of cookie approach where the
RsIterator asks the RowReader to index the columns and then hands that index back to the RowReader
on the readObjectArrayFrom method.  So I added two method signatures to RowReader:

    public int[] index(ResultSet rs);
    public void readObjectArrayFrom(ResultSet rs, int[] index, Map row);

and changed RowReaderDefaultImpl, RsIterator and SqlBasedRsIterator appropriately.  The other
alternative would have been to instantiate a RowReader for each RsIterator and keep this information
inside the RowReader itself, but that instantiation would have had a cost as well.  I left
the original readObjectArrayFrom() signature as well and left the PersistenceBroker.getObjectByIdentity()
path using it since for single item queries there is probably a very small performance advantage
to the original method.

Files modified:
   RowReader - added methods index and readObjectArrayFrom w/ index
   RowReaderDefaultImpl - implemented index and readObjectArrayFrom w/ index
   RsIterator - call RowReader to build index for ResultSet in constructor and call new readObjectArrayFrom
   SqlBasedRsIterator - call RowReader to build index for ResultSet in constructor

2) Three methods on ClassDescriptor, getFieldDescriptorByName, getObjectReferenceDescriptorByName,
and getCollectionDescriptorByName all have the same fundamental problem.  They each try to
build a descriptor by name map lazily.  To do this they look for the name in the map and if
they don't find it scan through all the descriptors and if found add it to the map.  The problem
with this is that queries for things where the descriptor doesn't exist always end up scanning
the entire set of descriptors in order to determine that it doesn't exist.  In other words
this strategy works fine for positive queries but is very poor for negative queries.  It turns
out that a fair number of negative queries do occur though for things like the field descriptor
for OJB_CONCRETE_CLASS.  I also fixed a bug where ClassDescriptor doesn't rebuild it's cached
lists of FieldDescriptors when new field descriptors are added and I cached the result of

Files modified:
      - changed map building strategy in getXXXDescriptorByName methods
      - changed setClassOfObject and isAbstract to cache value of isAbstract
      - changed addFieldDescriptor to clear out cached lists of field descriptors and clear
the name map
      - changed addCollectionDescriptor and addObjectReferenceDescriptor to clear their respective
name maps 

3) ObjectCacheDefaultImpl and ObjectCachePerBrokerImpl both call toString() on the Identity
passed in and use that as the key to their internal maps.  In addition Identity calls toString()
on itself to calculate it's hashCode.  Converting the Identity objects to Strings turns out
to be much more costly than simply comparing their contents.  Compounding matters is the fact
that a lot of Identity objects get constructed solely to do cache lookups and then are immediately
discarded but they get unnecessarily converted to String in the process at extra cost in processing
and object creation.  I changed the two caches to use the Identity object itself as the key
and changed Identity.hashCode() to calculate the hashCode directly rather than converting
to a String.

Files modified:
   Identity - changed hashCode() method
   ObjectCacheDefaultImpl - removed all the toString()'s on Identity objects
   ObjectCachePerBrokerImpl - ditto

4) LoggerFactoryImpl.getLogger concatenates a bunch of Strings for debug messages without
checking to see if debug verbosity is enabled.  I protected all the calls with an isEnabledFor(Logger.DEBUG)

Files modified:
   LoggerFactoryImpl - check isEnabledFor(Logger.DEBUG) before doing String concatenations

5, 6 and 7) PersistenceBrokerImpl is creating iterators to iterate through Vectors returned
from ClassDescriptor methods.  I changed it to get elements by index instead (which is significantly
faster).  Even better would be if the ClassDescriptor methods returned arrays of the appropriate
type instead of Vectors (this would be more consistent with the rest of the ClassDescriptor
interface as well as performing better and being more type-safe), but this would've required
an interface change to ClassDescriptor.

Also a lot of logger.debug messages were added to getRsIteratorFromQuery that are doing String
concatenations.  I protected all these calls with logger.isDebugEnabled() checks.

When returning a collection proxy PersistenceBrokerImpl/ProxyHelper always looks up the constructor
again each time though the answer never changes.  I changed it to remember the constructors
and use those instead of looking up the constructors from the classes each time.

Files modified:
   PersistenceBrokerImpl - replaced iterators with get elements by index, remembered proxy
collection constructors and protected logger.debug messages
   ProxyHelper - remember constructors for standard proxy collections

8) SqlHelper.cleanPath calls splitPath.  This results in lots of PathInfo objects being created
needlessly and immediately discarded because cleanPath is only interested in the column portion.
 I changed it to directly calculate the column portion itself and to avoid two indexOf operations
in both cleanPath and splitPath when unnecessary.

Files modified:
   SqlHelper - optimized cleanPath and splitPath

9) PersistenceBrokerAbstractImpl manages firing off events during the lifecycle of a PersistenceBroker.
 There are three opportunities for performance improvements here.  First it iterates through
a map every time an event is fired.  Since listeners are added rarely but events are fired
frequently it's much better to construct a new array each time a listener is added and iterate
through an array on event firing.  Second, listeners are stored in a map with a Boolean indicating
whether they are permanent or not.  When the broker is released this map is iterated through
and all non-permanent listeners are discarded.  It's better to keep two separate arrays and
just discard the non-permanent array rather than doing this iteration.  Finally, a single
fireBrokerEvent method is provided which takes a PersistenceBrokerEvent.  Internally this
method does two instanceof operations to decide how to dispatch the event.  Letting the compiler
decide how to dispatch at compile time by providing overloaded signatures of fireBrokerEvent
is more efficient.  Similar things could be done for add and removeListener, but I didn't
bother since these will be called relatively rarely.

Files modified:
   PersistenceBrokerAbstractImpl - changed storage of listeners to arrays instead of Maps,
added overloaded fireBrokerEvent methods
   PersistenceBroker - added overloaded fireBrokerEvent methods
   DelegatingPersistenceBroker - added overloaded fireBrokerEvent methods
   PersistenceBrokerBean - added overloaded fireBrokerEvent methods

10) ObjectReferenceDescriptor.getItemProxyForClass uses whether the proxy is null or not as
a test for whether it's been looked up.  For classes that don't have a proxy though the proxy
will always be null so it will do that lookup again every time the method is called.  I changed
it to have a boolean indicating whether the proxy had been looked up instead.

Files modified:
   ObjectReferenceDescriptor - changed getItemProxyForClass to use a boolean to indicate whether
the proxy has been looked up

View raw message